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
cleanup helm chart #16896
cleanup helm chart #16896
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.
Thanks very much, overall this looks good to me!
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.
remove redundant
namespace: {{ .Release.Namespace }}
I mentioned it in the other PR, but this is unfortunately not redundant. This is needed for correct use of helm template --namespace
, see helm/helm#3553 (comment)
We are relying on the fact that one can kubectl create
the generated YAML without an explicit -n
in the docs, so this will break existing workflows.
Thank @gandro for clearify. |
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.
From a first glance, this looks correct now, thank you! It will require some manual testing to ensure there are no significant differences in the output.
One thing we need to document (because it's a breaking change) is the removal of the Capabilities
values. Please add an upgrade note here:
https://github.com/cilium/cilium/blob/master/Documentation/operations/upgrade.rst#111-upgrade-notes
It could be something like:
- The
Capabilities
Helm value has been removed. When usinghelm template
to generate the Kubernetes manifest for a specific Kubernetes version, please use the--kube-version
flag (introduced in Helm 3.6.0) instead.
|
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.
Starting to look good. A few minor things left that I noticed while comparing the helm template
output
install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml
Show resolved
Hide resolved
install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml
Show resolved
Hide resolved
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 overall, although my knowledge of the Helm chart is limited and I'm not able to provide an in-depth review of the changes.
Here are a few nitpicks below. In addition,
- Please make sure to update the Helm reference in the Documentation (see comment below),
- And please update your commit log message regarding the
remove redundant namespace: {{ .Release.Namespace }}
, given that, if I understand the discussion with Sebastian, this is in fact not the case.
install/kubernetes/cilium/templates/cilium-agent/clusterrole.yaml
Outdated
Show resolved
Hide resolved
install/kubernetes/cilium/templates/cilium-preflight/clusterrole.yaml
Outdated
Show resolved
Hide resolved
install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml
Outdated
Show resolved
Hide resolved
Travis on ARM failed with a transient error, should not block this PR: https://app.travis-ci.com/github/cilium/cilium/jobs/526175793
|
test-me-please |
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. Could you squash Update some typo
into the first commit? It fits the description of the first commit anyway.
Looks like you'll also need to rebase on master.
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.
A few more git history related comments.
{{- if .Values.operator.resources }} | ||
{{- with .Values.operator.resources }} | ||
resources: | ||
{{- toYaml .Values.operator.resources | trim | nindent 10 }} | ||
{{- toYaml . | trim | nindent 10 }} |
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.
This change is supposed to be part of the 1st commit but is part of the 3rd?
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.
@christarazi The 1st commit also contains priorityClassName
changes, so I decided to squash 3rd commit into 1st commit.
I also update 1st commit message to match w/ this PR description.
{{- if .Values.imagePullSecrets }} | ||
{{- with .Values.imagePullSecrets }} | ||
imagePullSecrets: | ||
{{- toYaml .Values.imagePullSecrets | nindent 6 }} | ||
{{- toYaml . | nindent 6 }} |
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.
This change is supposed to be part of the 1st commit but is part of the 3rd?
|
||
{{- /* Workaround so that we can set the minimal k8s version that we support */ -}} | ||
{{- $k8sVersion := .Capabilities.KubeVersion.Version -}} | ||
{{- $k8sMajor := .Capabilities.KubeVersion.Major -}} | ||
{{- $k8sMinor := .Capabilities.KubeVersion.Minor -}} | ||
|
||
{{- if .Values.Capabilities -}} | ||
{{- if .Values.Capabilities.KubeVersion -}} | ||
{{- if .Values.Capabilities.KubeVersion.Version -}} | ||
{{- $k8sVersion = .Values.Capabilities.KubeVersion.Version -}} | ||
{{- if .Values.Capabilities.KubeVersion.Major -}} | ||
{{- $k8sMajor = toString (.Values.Capabilities.KubeVersion.Major) -}} | ||
{{- if .Values.Capabilities.KubeVersion.Minor -}} | ||
{{- $k8sMinor = toString (.Values.Capabilities.KubeVersion.Minor) -}} | ||
{{- end -}} | ||
{{- end -}} | ||
{{- end -}} | ||
{{- end -}} | ||
{{- end -}} | ||
|
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.
This change is supposed to be part of the 1st commit but is part of the 3rd?
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.
Thank you for this amazing PR. Two comments:
- Is there any linter that you used to catch all of these inconsistencies?
- Just to confirm, all changes from install/kubernetes: add priorityClasses #16933 seem to be included in this PR, correct?
|
test-me-please |
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.
🚀
* remove .Values.Capabilities in favor of --kube-version in helm template * using `cilium.image` template to unify image render * support `priorityClassName` in all components * move `hubble-metrics` service & `hubble` ServiceMonitor to seperated file (in hubble folder) * make `name` as the first attribute of object * remove unnecessary parentheses * using `nindent` instead of `indent` * using `default` and `ternary` instead of `if-else` * using `with` instead of `if` when it's possible Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
@christarazi I rebased from master. But seem |
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.
Not sure why you need another review from me, but it still looks good.
@qmonnet sorry, I click to request review from @errordeveloper but accident click into your name :) |
test-me-please |
Ilya is currently out of office, given that his feedback has been addressed I think we can take his earlier comment as an approval. I'm unassigning him. |
Considering Ilya's implicit approval (see above) and that all tests have passed, I'm labelling as |
This is next part of refactoring helm chart #16792 with the following changes:
remove
.Values.Capabilities
in favor of--kube-version
in helm template, as discussion in #16752Unify image using
cilium.image
template.cilium/install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml
Line 226 in c95f6fd
Currently, configuring image is quite long, and not consistent between components (some support digest, some are not). By using
cilium.image
template, it's make configuring image shorter, easier to read and consistent between components.support
priorityClassName
.Currently, some components (e.g agent) support define
priorityClassName
in values file, but not using in template :|, other components (e.g hubble-ui, hublle-relay) are not supportpriorityClassName
. So, with this MR,priorityClassName
is supported in all components.move
hubble-metrics
service &hubble
ServiceMonitor to seperated file (inhubble
folder)remove unnecessary parentheses
make
name
as the first attribute of objectusing
nindent
instead ofindent
using default and ternary instead of if-else
using
with
instead ofif
when it's possibleSigned-off-by: Đặng Minh Dũng dungdm93@live.com