-
Notifications
You must be signed in to change notification settings - Fork 125
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
remove non-root overlays #3404
remove non-root overlays #3404
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.
Looks good!
Do we currently have test coverage for this?
This previously broke silently because we were not checking that every component rolled out successfully.
Could we write a test that does this?
Happy to work with you to test this
@daxmc99 we currently don't...for any of our overlays. We have an issue here to test upgrades for all of our overlays. I think we should properly define the scope of that issue to include testing before the upgrade and after. Thoughts? Happy to discuss and rescope that issue, and determine exactly what defines a successful deployment and how we test that. |
Merging this as is, with the intention that any testing be done as part of the previously mentioned issue. That's a high priority in the backlog so will get pulled into the next sprint. |
This PR removes the non-root overlay, as it does not deploy correctly without roles/clusterroles and is largely redundant given the non-privileged overlay provides a the roles we would ideally use in a secure cluster anyway.
Fixes https://github.com/sourcegraph/sourcegraph/issues/19724