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.

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

feat(argocd-image-updater): Make cm and secret names configurable #2998

Merged
merged 6 commits into from
Jan 25, 2025

Conversation

KyriosGN0
Copy link
Contributor

@KyriosGN0 KyriosGN0 commented Oct 29, 2024

Signed-off-by: AvivGuiser [email protected]

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).

@mkilchhofer
Copy link
Member

Hi @KyriosGN0
May I ask for your motivation to template the ConfigMap/Secret?
Especially the two resources argocd-image-updater-secret and argocd-image-updater-config should be "hardcodeded" since we strictly follow the upstream projects.

And it is mentioned several times inside AUI's documentation

@KyriosGN0
Copy link
Contributor Author

KyriosGN0 commented Oct 30, 2024

we have a single argocd instance but we have 3 different image updaters, i don't want them to share the same secret/cm as they supposed to be managed in argo and if three different applications will try to control the same cm it won't work
@mkilchhofer

@KyriosGN0 KyriosGN0 force-pushed the feat/imge-updater-cm-name branch from 05a097b to 40db0db Compare November 5, 2024 17:42
@github-actions github-actions bot added the size/M label Nov 5, 2024
@KyriosGN0
Copy link
Contributor Author

@mkilchhofer i understand that you follow the upstream project, however i belive that in this case we preserve the purpose of those cm by adding the suffixes to the end of the templated name

@mkilchhofer
Copy link
Member

@mkilchhofer i understand that you follow the upstream project, however i belive that in this case we preserve the purpose of those cm by adding the suffixes to the end of the templated name

My only concern is that the name changes depending on the helm command our users are using (helm's release-name). E.g.:

  • helm install aui argo/argocd-image-updater -> with your implementation, the CMs are then named:
    • aui-argocd-image-updater
    • aui-argocd-image-updater-sshconfig
    • aui-argocd-image-updater-config
  • helm install image-updater argo/argocd-image-updater -> with your implementation, the CMs are then named:
    • image-updater-argocd-image-updater
    • image-updater-argocd-image-updater-sshconfig
    • image-updater-argocd-image-updater-config

Maybe using 3 top-level variables with a static name would fit better? Opinions, @yu-croco @jmeridth @tico24 @mbevc1 ?

@KyriosGN0
Copy link
Contributor Author

@mkilchhofer im probably missing something, what is the issue with those names being different ? the deployment will just pull the new names (with their data unchanged)

@KyriosGN0
Copy link
Contributor Author

Hey @mkilchhofer is there any info I can give to expedite the decision ?

@KyriosGN0
Copy link
Contributor Author

hey @mkilchhofer any updates? sorry for nagging

@mbevc1
Copy link
Collaborator

mbevc1 commented Dec 3, 2024

@mkilchhofer i understand that you follow the upstream project, however i belive that in this case we preserve the purpose of those cm by adding the suffixes to the end of the templated name

My only concern is that the name changes depending on the helm command our users are using (helm's release-name). E.g.:

* `helm install aui argo/argocd-image-updater` -> with your implementation, the CMs are then named:
  
  * **aui**-argocd-image-updater
  * **aui**-argocd-image-updater-sshconfig
  * **aui**-argocd-image-updater-config

* `helm install image-updater argo/argocd-image-updater` -> with your implementation, the CMs are then named:
  
  * **image-updater**-argocd-image-updater
  * **image-updater**-argocd-image-updater-sshconfig
  * **image-updater**-argocd-image-updater-config

Maybe using 3 top-level variables with a static name would fit better? Opinions, @yu-croco @jmeridth @tico24 @mbevc1 ?

Oh, totally missed this tag 😄

I like that suggestion and makes sense @mkilchhofer .

@KyriosGN0
Copy link
Contributor Author

hey @mkilchhofer, sorry for nagging again, are there any updates? anything i should do ?

@KyriosGN0
Copy link
Contributor Author

Hey @mkilchhofer , another hopeful ping

@KyriosGN0 KyriosGN0 force-pushed the feat/imge-updater-cm-name branch from 71c69c7 to 94947f2 Compare January 18, 2025 09:04
Signed-off-by: AvivGuiser <[email protected]>
@mkilchhofer mkilchhofer force-pushed the feat/imge-updater-cm-name branch from 599db6c to ece3f9b Compare January 23, 2025 23:18
@mkilchhofer mkilchhofer force-pushed the feat/imge-updater-cm-name branch from ece3f9b to c26278d Compare January 23, 2025 23:23
@mkilchhofer
Copy link
Member

@KyriosGN0 @yu-croco @jmeridth @mbevc1
I implemented a suggestion in c26278d. Let me know what you think :)

@mbevc1
Copy link
Collaborator

mbevc1 commented Jan 24, 2025

Thanks @mkilchhofer - looks good!

@mkilchhofer mkilchhofer requested a review from tico24 January 24, 2025 13:07
@mkilchhofer
Copy link
Member

@tico24 WDYT? :) are you okay with this?

mkilchhofer
mkilchhofer previously approved these changes Jan 24, 2025
tico24
tico24 previously approved these changes Jan 24, 2025
Copy link
Member

@tico24 tico24 left a comment

Choose a reason for hiding this comment

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

Approved but you'll need to rebase.

@mkilchhofer mkilchhofer dismissed stale reviews from tico24 and themself via 290a556 January 24, 2025 20:11
@mkilchhofer mkilchhofer changed the title feat(argocd-image-updater): made argocd image updater cm and secret name templated feat(argocd-image-updater): Make cm and secret names configurable Jan 24, 2025
@mkilchhofer mkilchhofer requested a review from tico24 January 24, 2025 21:13
@mbevc1 mbevc1 merged commit 79ec8fd into argoproj:main Jan 25, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants