-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix(db-operator/rbac): Add missing list and watch for deployments for custom resource monitoring #33
Conversation
I'm a bit confused. It's failing to list deployments, but I don't see when you need it. You're using google instances, not generic, right? @hcnp |
Yes. It's on Google Cloud SQL. It's mainly these lines where I guess the error is "watch":
So I'm actually only changing this file: charts/db-operator/templates/controller/rbac.yaml The other changes is to create a template for updating the chart readme.md file with this tool: https://github.com/norwoodj/helm-docs. I can move that to a seperate PR. |
It's already implemented here: #16 |
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.
So, since I'm not using GSQL instances and I'm not actually the one who's been developing that part, it seems fine to me to add those permissions, though I don't understand what exactly introduced that bug and how tests are passing. I guess we want to re-work permissions later anyway.
I would ask you to drop readme related changes and leave only RBAC and Chart.yaml files updated.
I'd say that after this one is merged: #32, it makes sense to rebase and bump a patch version.
@hcnp Woud you be able to apply those fixes and rebase to the main? |
fbce703
to
8ecd6ca
Compare
I've rebased. I've also tested this again. Without the "list" permission the status of the db resource won't get updated correctly in addition to the error and log lines above, but the dbuser and db gets created correctly together with the gcloud proxy:
|
@hcnp We'll unfortunately have to wait for #43 and rebase again. But to get something merged to the charts repo, if you update charts, you always need to bump a new chart version. Since I see that there is only one file changed, I think you still need to do it. It seems like a fix to me, so I would go with a patch |
Please, rebase one more time :) |
The operator needs this to monitor the custom resources.
there is a conflict, and current dbin version is 2.2.0: https://github.com/db-operator/charts/blob/main/charts/db-instances/Chart.yaml We've just released a big change for a test pipieline a couple of days ago and dbinsntances were affected too |
cb85c7b
to
e9f4915
Compare
Done ;) |
e9f4915
to
6bb881e
Compare
6bb881e
to
3e8cd1b
Compare
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
The operator needs this to monitor the custom resources.
Without these permissions the operator will fail to monitor custom resources and thus fail to update them.
This was in the operator log before the change: