- Notifications
You must be signed in to change notification settings - Fork914
chore: refactor entitlements to be a safe object to use#14406
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Previously, all usage of entitlements requires mutex usage on theapi struct directly. This prevents passing the entitlements toa sub package. It also creates the possibility for misuse.
| "github.com/coder/coder/v2/codersdk" | ||
| ) | ||
| type Set struct { |
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 implemented the methods as I saw them used. There might be a way to reduce the number of methods on this struct.
Uh oh!
There was an error while loading.Please reload this page.
| return f, ok | ||
| } | ||
| func (l *Set) Enabled(feature codersdk.FeatureName) bool { |
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.
Potential follow-up: we could replace this withf, ok := Features(name); ok && f.Enabled?
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.
True. Because before we had access to the whole struct, our usage of it seemed a bit arbitrary at times. Sometimes we grab it and check entitled, most times just enabled.
I'm not trying to fix all our usage right now, but it would be good to audit at some times.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
missing tests for the rest of the Entitlements methods
LGTM pending some more tests, but approving pre-emptively. |
af125c3 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Previously, all usage of entitlements requires mutex usage on the
api struct directly. This prevents passing the entitlements to
a sub package. It also creates the possibility for misuse.
I am refactoring this because I want to place some IDP sync code into it's own package.
I'd prefer if the IDP code handled the entitlements (it's enterprise code).