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

feat(argo-cd): support ApplicationSet in any namespace. #2402

Merged
merged 3 commits into from
Mar 2, 2024
Merged

feat(argo-cd): support ApplicationSet in any namespace. #2402

merged 3 commits into from
Mar 2, 2024

Conversation

mugioka
Copy link
Contributor

@mugioka mugioka commented Dec 25, 2023 •
edited by yu-croco
Loading

Resolves #2293

I have added ClusterRole/ClusterRoleBinding for applicationset-controller to support ApplicationSet in any namespace.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch 2 times, most recently from 7785854 to 924b3c0 Compare December 25, 2023 08:26
@mugioka mugioka marked this pull request as draft December 25, 2023 09:22
yu-croco
yu-croco reviewed Dec 25, 2023
View reviewed changes
charts/argo-cd/templates/argocd-applicationset/clusterrole.yaml Outdated
@@ -0,0 +1,32 @@
{{- if .Values.createClusterRoles }}
Copy link
Collaborator

@yu-croco yu-croco Dec 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not all users use ApplicationSet in any namespace feature, so I think it's better to add a new flag. WDYT?🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so.
However, to enable the application in any namespace feature, we must set .Values.createClusterRoles=true or .Values.controller.clusterAdminAccess=true ( .controller.clusterAdminAccess is deprecated). .
Therefore, we believe it is good to be able to enable application set in any namespace feature in the same way (since those who use application in any namespace are also likely to use application set in any namespace).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I submitted a PR to argo-cd repo (argoproj/argo-cd#16699).
After the above PR is merged, it would be better to merge this PR.

Copy link
Collaborator

@yu-croco yu-croco Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you !
Let us add await-upstreaming label at this PR to wait upstream's PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argoproj/argo-cd#16699 is merged!

Please re-review.
@yu-croco

Copy link
Member

Choose a reason for hiding this comment

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

@yu-croco We can probably review this - 2.10.0 is released and this change will be cherry-picked by upstream for next release patch release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pdrastil
Thank you for the info 👍

@mugioka
When conflict is resolved, I will approve it. 🙋

Copy link
Member

Choose a reason for hiding this comment

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

Can we add an and expression with a new value (eg. .Values.applicationSet.allowAnyNamespace or .Values.features.applicationSetInAnyNamespace)

This feature needs to be disabled by default IMHO. We at Swiss Post don't want to allow this and also this feature is beta.
Ref: https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/Appset-Any-Namespace/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Can we add an and expression with a new value (eg. .Values.applicationSet.allowAnyNamespace or .Values.features.applicationSetInAnyNamespace)

i will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yu-croco
Sorry for delay,,,

I fixed feedback.
Please re-review this pr.

@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch from 924b3c0 to 2f3d5e7 Compare December 26, 2023 02:19
@github-actions github-actions bot added size/L and removed size/M labels Dec 26, 2023
@mugioka mugioka marked this pull request as ready for review December 26, 2023 02:21
This was referenced Dec 26, 2023
Closed
Merged
mugioka added a commit to mugioka/argo-cd that referenced this pull request Dec 26, 2023
@mugioka
chore(manifests): add ClsuterRole/ClusterRoleBinding for applications…
68348c9 
…et controller.

Closes argoproj/argo-helm#2402.

Signed-off-by: mugioka <okamugi0722@gmail.com>
@yu-croco yu-croco added the awaiting-upstream Is waiting for a change upstream to be completed before it can be merged. label Dec 26, 2023
@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch from 1ffb117 to b17ca8c Compare January 30, 2024 06:42
@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch from b17ca8c to 1ffb117 Compare January 30, 2024 06:43
@pdrastil pdrastil changed the title chore(argo-cd): support ApplicationSet in any namespace. feat(argo-cd): support ApplicationSet in any namespace. Feb 11, 2024
@mugioka
chore(argo-cd): support ApplicationSet in any namespace.
d46b304 
Signed-off-by: mugioka <okamugi0722@gmail.com>
@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch 2 times, most recently from 15e1c60 to e278087 Compare March 2, 2024 05:53
@mugioka
chore: apply feedback by maintainer.
1c173f4 
Signed-off-by: mugioka <okamugi0722@gmail.com>
@mugioka mugioka force-pushed the chore/support-applicationset-in-any-namespace branch from e278087 to 1c173f4 Compare March 2, 2024 05:55
@mugioka
fix: docs is not generated.
1de724a 
Signed-off-by: mugioka <okamugi0722@gmail.com>
yu-croco
yu-croco approved these changes Mar 2, 2024
View reviewed changes
Copy link
Collaborator

@yu-croco yu-croco left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. LGTM. :)

@mbevc1 mbevc1 merged commit de462b7 into argoproj:main Mar 2, 2024
6 checks passed
@mugioka mugioka deleted the chore/support-applicationset-in-any-namespace branch April 26, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbevc1 mbevc1 mbevc1 approved these changes

@yu-croco yu-croco yu-croco approved these changes

@mkilchhofer mkilchhofer Awaiting requested review from mkilchhofer mkilchhofer is a code owner

@jmeridth jmeridth Awaiting requested review from jmeridth jmeridth is a code owner

@pdrastil pdrastil Awaiting requested review from pdrastil pdrastil is a code owner

@tico24 tico24 Awaiting requested review from tico24 tico24 is a code owner

@vladlosev vladlosev Awaiting requested review from vladlosev

Assignees
No one assigned
Labels
argo-cd awaiting-upstream Is waiting for a change upstream to be completed before it can be merged. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ApplicationSet ClusterRole Resource
6 participants
@mugioka @mkilchhofer @pdrastil @mbevc1 @yu-croco @tico24