Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Expose operator azure-user-assigned-identity-id flag to its chart #13424

Merged
merged 1 commit into from Oct 20, 2020
Merged

Expose operator azure-user-assigned-identity-id flag to its chart #13424

merged 1 commit into from Oct 20, 2020

Conversation

ombre9
Copy link

@ombre9 ombre9 commented Oct 6, 2020 •
edited

Exposing Cilium Operator's flag --azure-user-assigned-identity-id to its chart.

Signed-off-by: Andor Nemeth ombre9@gmail.com

@ombre9 ombre9 requested review from a team as code owners October 6, 2020 12:53
@ombre9 ombre9 requested a review from a team October 6, 2020 12:53
@maintainer-s-little-helper
Copy link

Commit e8958dcde030e8725efc1ccb824d9ed1fd2f8288 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 6, 2020
@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Oct 6, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 6, 2020
@pchaigno pchaigno added area/operator Impacts the cilium-operator component area/helm Impacts helm charts and user deployment experience labels Oct 6, 2020
joestringer
joestringer requested changes Oct 6, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Thanks for the contribution!

As the maintainer's little helper describes above, it is the project's policy to ensure that contributors sign off their work to certify the origin of the contributions. See the links above for more details.

I've left a more specific note about the changes themselves below, it would be good to define this option consistently with the other options in the configmap instead of in the deployment.

install/kubernetes/cilium/charts/operator/templates/deployment.yaml Outdated
Comment on lines 57 to 59
{{- if .Values.global.azure.userAssignedIdentityID }}
- --azure-user-assigned-identity-id={{ .Values.global.azure.userAssignedIdentityID }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is currently implemented in the deployment, but the --config-dir option above ensures that the cilium-config configmap options will be made available to the operator. You should be able to implement this instead by adding a new key/value to the configmap with the name azure-user-assigned-identity-id instead, which will keep the deployment templates here separate from where all the config options are declared (currently install/kubernetes/cilium/charts/config/templates/configmap.yaml).

Additionally, to let you know, PR #13259 is about to do some significant refactoring of the files that declare these settings to avoid declaring all options as global. We anticipate to merge that PR this week, in which case the changes for this PR would need to apply to a different file location.

@maintainer-s-little-helper
Copy link

Commit e8958dcde030e8725efc1ccb824d9ed1fd2f8288 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 7, 2020
@aanm aanm added conflicts-with-pr dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. needs/triage This issue requires triaging to establish severity and next steps. and removed conflicts-with-pr dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed labels Oct 9, 2020
@aanm
Copy link
Member

aanm commented Oct 13, 2020

@ombre9 can you rebase your PR? Thank you!

@aanm aanm added release-priority/best-effort The project for target version is not a hard requirement. and removed needs/triage This issue requires triaging to establish severity and next steps. labels Oct 13, 2020
Expose operator azure-user-assigned-identity-id flag to its chart
2df931e 
Signed-off-by: Andor Nemeth <ombre9@gmail.com>
@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 15, 2020
@aanm
Copy link
Member

aanm commented Oct 15, 2020

test-me-please

@aanm aanm requested a review from joestringer October 15, 2020 13:19
@joestringer joestringer merged commit ad8f051 into cilium:master Oct 20, 2020
@nathanjsweet nathanjsweet mentioned this pull request Oct 22, 2020
Merged
25 tasks
@joestringer joestringer mentioned this pull request Nov 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. release-priority/best-effort The project for target version is not a hard requirement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ombre9 @aanm @joestringer @nathanjsweet @pchaigno