-
Notifications
You must be signed in to change notification settings - Fork 99
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 RBAC rules for operator so it can manage users #1527
Conversation
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.
Could you update the CHANGELOG before merging?
charts/operator/chart_test.go
Outdated
@@ -234,6 +234,10 @@ const preTranspilerChartVersion = "0.4.28" | |||
// TestChartDifferences can be removed if in the next operator chart version values definition changes or any resource. | |||
// That test only validates clean transition to gotohelm definition of the operator helm chart. | |||
func TestChartDifferences(t *testing.T) { |
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 think we're safe to remove this test now as we've finished the transition to gotohelm.
Think it would be worth it to add a test that runs Kustomize against the appVersion
of the operator in the chart and checks for divergences of RBAC?
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.
Agree. It was just for me to validate I'm not breaking chart all over the places.
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.
Think it would be worth it to add a test that runs Kustomize against the appVersion of the operator in the chart and checks for divergences of RBAC?
I'm thinking that the problem with that is that we still might be out-of-date if we have an unreleased operator. I think the only sure-fire way is if we start failing if say we're missing RBAC stuff that is currently in main
.
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 thinking that the problem with that is that we still might be out-of-date if we have an unreleased operator.
Isn't that fine? Oh, right we run this chart in the operator test suite don't we 😮💨 .
I think the only sure-fire way is if we start failing if say we're missing RBAC stuff that is currently in main.
Then we'll have the opposite problem of tests failing until we update the operator. I like to avoid tests that suddenly fail based on external dependencies changing when possible.
Let's backlog something to remove the circular dependency we have?
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.
👍 another reason I'd love to merge some of this into the same codebase 😅
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.
Come to think of it, the operator chart might be a great first attempt at having the helm charts just follow another repository given that all the sources of truth at in the operator repo.
@@ -4,6 +4,7 @@ | |||
|
|||
### [Unreleased](https://github.com/redpanda-data/helm-charts/releases/tag/redpanda-FILLMEIN) - YYYY-MM-DD | |||
#### Added | |||
* Add RBAC rules for the operator chart so it can manage users |
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 approved, but this should be moved to operator chart section. In the #1530 I'm fixing that
This adds the RBAC rules necessary for the operator to manage the new User resource we're adding. In order for this to pass tests I had to skip the
TestChartDifferences
test since that ensures that the same manifests render between operator releases, which is not the case here where we're adding some stuff to the operator manifests.