-
Notifications
You must be signed in to change notification settings - Fork 707
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
Moved global repos to their own namespace and outside the kubeapps one #3990
Changes from 5 commits
49bbf7a
0e0e491
8829abf
1258485
6883181
4581daa
ca1d2ca
916bd8f
f681ee5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,7 @@ spec: | |
{{- end }} | ||
- --repo-sync-cmd=/asset-syncer | ||
- --namespace={{ .Release.Namespace }} | ||
- --global-repos-namespace={{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferred to add a proper flag instead of passing the value through env variables There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +100. |
||
{{- if .Values.postgresql.existingSecret }} | ||
- --database-secret-name={{ .Values.postgresql.existingSecret }} | ||
{{- else }} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
apiVersion: v1 | ||
kind: Namespace | ||
metadata: | ||
name: "{{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Creates the global repos namespace |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -719,6 +719,9 @@ apprepository: | |
## - myRegistryKeySecretName | ||
## | ||
pullSecrets: [] | ||
## @param apprepository.globalReposNamespaceSuffix Suffix for the namespace of global repos | ||
## | ||
globalReposNamespaceSuffix: repos-global | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with the rest of the comments in that the suffix should also include the Also, in order to stick close to similar names within our organization, why not go with a similar suffix as Carvel people does: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find using the word |
||
## @param apprepository.initialRepos [array] Initial chart repositories to configure | ||
## e.g: | ||
## initialRepos: | ||
|
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.
Regarding making this a non-breaking change, could we instead do:
so that if
globalReposNamespaceSuffix
is empty (which can be the default, initially), we will have the existing behavior? We can then update the default value to-repos-global
when we do a major version change, breaking the backwards compatibility?It'd require a conditional around the namespace creation later too, etc.
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.
Thanks for the suggestion Michael, it makes sense. So, defaulting now to the same
kubeapps
namespace, would mean leaving without effect the purpose of this change until next major version 3.x?As-is in a Kubeapps upgrade scenario, new global repos will go to the new ns and old global repos will stay in the
kubeapps
ns, just not being considered global anymore.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.
By default, yes, but we can verify it works as expected, even update CI to use it (by setting a value for the suffix) if we want, to ensure that switching the default later is a non-event (other than the instructions for users).
Yep, we'll just ensure we document that in the release notes so people can move their repos as needed.