-
Notifications
You must be signed in to change notification settings - Fork 618
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
[charts/occm] Implement imagePullSecret support for master branch #2446
[charts/occm] Implement imagePullSecret support for master branch #2446
Conversation
Hi @carlotardl. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thank you! Please note that the linter is currently failing because you did not bump the chart version in Chart.yaml. It would be fantastic if you could incorporate that here and squash/fixup your commits into a single one. |
/ok-to-test |
/lgtm please squash the commits before merge .. |
seems you need a bump after rebase your code
|
5900313
to
a0cf66d
Compare
a0cf66d
to
958f415
Compare
It's been rebased, I had different sources and there was a mixup but I updated it on the last commit |
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 much better, thanks. I was wondering if we'd like to treat this setting similar to others in that template.
charts/openstack-cloud-controller-manager/templates/daemonset.yaml
Outdated
Show resolved
Hide resolved
charts/openstack-cloud-controller-manager/templates/daemonset.yaml
Outdated
Show resolved
Hide resolved
958f415
to
5f5309c
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.
Thanks, I am sure that the functionality to reference imagePullSecrets
will come in handy in a variety of contexts.
I've added a couple of inline comments and just want to make sure that this works as desired in the variety of use-cases users would want to implement.
Furthermore, I am curious if we want to consider adding this to the ServiceAccount
instead or let users choose one approach over the other.
The former ties closer in how access through workload identity is typically implemented and might rather be what users expect.
See https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/ for details.
@@ -10,6 +10,7 @@ commonAnnotations: {} | |||
# "helm.sh/hook-delete-policy": before-hook-creation | |||
|
|||
# Image repository name and tag | |||
imagePullSecrets: [] |
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 should probably be on the line above, to ensure that the comment # Image repository name and tag
remains in the right place.
Furthermore, it would be great to add a similar comment that details how to use imagePullSecrets
with an example.
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 was fixed, please re-check.
{{- with .Values.imagePullSecrets }} | ||
imagePullSecrets: | ||
{{- toYaml . | nindent 8 }} | ||
{{- 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.
I am not entirely sure this works with the default of imagePullSecrets: []
that is currently being set in the values file.
We have to make sure that this works in the following cases:
- Default case (user does not set
imagePullSecrets
and they areimagePullSecrets: []
- Single element in
imagePullSecrets
- Multiple elements in
imagePullSecrets
This might require a combination of if
, range
and/or with
to achive the desired outcome.
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.
With the values file, the output should be empty, but once we overwrite the values.yml with
imagePullSecrets:
- one
The output will be:
imagePullSecrets:
- one
It also works with more values, it can be tested with
helm template --values values.yaml .
I updated the last commit with a few comments.
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, tested with helm template and all possibilities render correctly.
5f5309c
to
addcab3
Compare
/test openstack-cloud-csi-manila-sanity-test |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jichenjc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-1.28 |
@wwentland: #2446 failed to apply on top of branch "release-1.28":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
This PR implements
imagePullSecrets
for theopenstack-cloud-controller-manager
helm chart on master branch.Which issue this PR fixes(if applicable):
None applicable
Special notes for reviewers:
Release note: