Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add config value that gives users options to skip calculating role for each lease #22651
Add config value that gives users options to skip calculating role for each lease #22651
Changes from 19 commits
ec9289e
e0d432f
e23a03d
373d912
bd03841
0fc4e3e
19b8aed
b6354c1
7567e35
ddc88bd
6447ffa
dfd08a8
679f0fd
17bff60
abe919f
eece07d
437ae3e
5537c96
69385fa
c97469d
f28eed5
85593b9
27ca6da
35ed801
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Though it's more verbose, how about
imprecise_lease_count_quota_role_tracking
?I worry that "lease role" won't mean a lot to customers, and changing the name in the future if it's confusing will be difficult. I also want to be very explicit that it's lease count quota specific, and doesn't affect lease behaviour otherwise.
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'm fine with this change. Although, I think documentation would help here the most. A lot of vault config names/env vars names are confusing to me until I read the docs
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.
wow I keep going back and forth on this one. The only concern I have is that
imprecise_lease_count_quota_role_tracking
is a bit of a mouthful. @ncabatoff @mpalmi do you have a preference?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.
If we want to go this route, perhaps
role_tracking
is too much of an implementation detail and we could get away withskip_pre_quota_lease_tracking
, orskip_old_lease_counts_on_quota_creation
?I'm honestly cool with any name we land on. This is a tricky one to pin down and I tend to agree that the documentation is the important part.
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'm going to stick with what we have. we have pretty good documentation on the option now which makes me feel better about it.