-
Notifications
You must be signed in to change notification settings - Fork 113
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
[auth] Update Condition Attribute enum values #1147
Conversation
75f6d35
to
279236c
Compare
:hrm: Are there any user-facing docs we'd need to update? Or concerns about backwards compatibility? |
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.
❤️ Love to see this kind of cleanup in the code! 💯
@@ -125,7 +125,7 @@ func nodeMatchesAllConditions(node backend.Node, conditions []*iam_v2.Condition) | |||
if !foundMatch { | |||
return false |
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.
(nothing to do with this PR just a question to myself and @lancewf)
Do we need this foundMatch
logic? why not just return true
in like 121
like the other functions? (I might be missing something)
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.
LGTM!
Signed-off-by: Brenna Hewer-Darroch <brenna@chef.io>
Signed-off-by: Brenna Hewer-Darroch <brenna@chef.io>
Signed-off-by: Brenna Hewer-Darroch <brenna@chef.io>
Signed-off-by: Brenna Hewer-Darroch <brenna@chef.io>
279236c
to
a564337
Compare
🔩 Description: What code changed, and why?
The allowed values for a rule's Condition Attribute were a mix of plural and singular and didn't match how they're meant to be shown in the UI. Now they've been updated to match this list provided by superteam:
👟 How to Build and Test the Change
✅ Checklist
📷 Screenshots, if applicable