-
Notifications
You must be signed in to change notification settings - Fork 118
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 resource namespaces saved during deploy to fallbackAllowedNamespaces #533
Conversation
5a2974d
to
8a695f1
Compare
How are we updating the |
a95f7e2
to
3bc581e
Compare
It uses a ServiceAccount with limited scope (no permission to list namespaces)
These resource namespaces are stored in app change meta during deploy
fe66c8e
to
4c741df
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.
Initial thoughts
I will take a closer look at how this affects how we list resources in allowedNamespaces
today
4c741df
to
2b8e038
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.
9c93dfb
to
1baaaab
Compare
Code looks good to me, gonna take a closer look at the new test! |
1baaaab
to
7968f8d
Compare
7968f8d
to
9288eb6
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.
What this PR does / why we need it:
Add resource namespaces saved during deploy to fallbackAllowedNamespaces.
These resource namespaces are stored as app change meta during deploy
Which issue(s) this PR fixes:
Fixes #479
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
I am still thinking if there's a better place to make this change. Another option that I thought of was to call the FactoryClients separately and a little late (after Exists function is called) and populate the fallbackAllowedNamespaces with the resource namespaces there itself, but that would require a lot of repetition as App.Find requires a lot of things from
FactoryClients
.Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: