-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set default access request TTL to 1 week. #35799
Conversation
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend rephrasing the title (changelog, commit message) as well. As written it's not clear as to whether you are describing the current behavior or the new behavior.
I like to complete the sentence "If applied, this commit will..."
For example: set default access request TTL to 24h.
cc1ce18
to
4365cbd
Compare
|
||
// requestTTL is the the TTL for an access request, i.e. the amount of time that | ||
// the access request can be reviewed. Defaults to 1 week. | ||
requestTTL = 7 * day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to override this TTL, or is it always 7 days no matter what?
If the latter, then it feels disingenuous to say this "Defaults to 1 week" - default typically means "the value that is used if not otherwise specified."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a way to override it with the maxDuration
flag. I'm not too familiar with this flag and how it works, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC we're here setting the object expiry (SetExpiry
) to 7 days. Below, we use maxDuration
to potentially modify SetSessionTTL
, SetAccessExpiry
, and SetMaxDuration
, but not SetExpiry
. Is there something I'm missing? If not, I think we should clarify the relationship between these settings and this maxDuration
before making these changes.
lib/services/access_request_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we getting rid of all of these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, my bad. I didn't restore the tests after restoring the function.
Hey all, if you've arrived here you're probably wondering why this is not yet merged. There's some complexity with the fact that the access request itself may expire before the access request TTL (the time to review) expires. We're currently trying to figure out what to do here. |
The TTL for a request now defaults to 1 week. This will allow reviewers more time to review an access request before it disappears.
ba91df6
to
358e527
Compare
I was going to merge this now that the rest of the bits are in place, but I'm going to do a bit more testing on second thought to make sure I didn't miss anything. |
I've tested this in combination with the other change and things seem to behave, so I'm going to merge/backport this. |
The TTL for a request now defaults to 1 week. This will allow reviewers more time to review an access request before it disappears.
changelog: Access Request TTLs default to 1 week.