fix: add username/password auth to helm/argocd autodiscovery - #9706
Conversation
Sorry, something went wrong.
|
Thank you for the pullrequets and the nice words, I'll try to review over the weekend |
Sorry, something went wrong.
|
@itsteddyyo Could you fix the linting issue? |
Sorry, something went wrong.
|
@olblak Sorry my bad! I did change some variable names and did not format properly 😄 Should be fixed now! Yeah i tested the generated binary locally and it worked as expected. I used both autodiscovery plugins but for the helm plugin i used ignorecontainer: true. But the changes should not affect that anyway. Thanks for taking a look! 😄 |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR updates the Helm and ArgoCD autodiscovery plugins so registry authentication details beyond tokens (username/password) are propagated into the generated Updatecli manifests, aligning behavior with what the docs describe.
Changes:
- Extend Helm dependency manifest generation to include username/password auth fields in addition to token.
- Extend ArgoCD manifest generation to include username/password auth fields in addition to token.
- Update ArgoCD autodiscovery spec/docs to represent auths as structured credentials rather than a token-only value.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file| File | Description |
|---|---|
| pkg/plugins/autodiscovery/helm/dependencyManifest.go | Adds username/password fields to the Helm dependency manifest template. |
| pkg/plugins/autodiscovery/helm/dependencies.go | Plumbs username/password from spec.Auths into dependency manifest template parameters. |
| pkg/plugins/autodiscovery/argocd/manifestTemplate.go | Adds username/password fields to the ArgoCD manifest template. |
| pkg/plugins/autodiscovery/argocd/main.go | Changes ArgoCD auth configuration shape to support structured credentials and updates docs/comments. |
| pkg/plugins/autodiscovery/argocd/application.go | Plumbs username/password from spec.Auths into manifest template parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Yes I agree, it's a good observation. |
Sorry, something went wrong.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
olblak
left a comment
There was a problem hiding this comment.
Thanks for the pullrequest
Sorry, something went wrong.
Hi! First of all, thanks for creating this great tool! 😃 It has become a big cornerstone of my update strategy.
While using it, I stumbled across an issue with some Autodiscovery plugins.
Both the Helm and ArgoCD Autodiscovery documentation mention support for username/password authentication in the auth section. However, it looks like only token is currently passed through by the code. This pull request adds support for passing both to the generated manifests as well.
I also fixed a small documentation issue where the ArgoCD auth example used <registry>: my-token instead of <registry>.token: my-token as I was already adding the other options anyway 😄
Test
To test this pull request, you can run the following commands:
I didn't add any additional tests because passing the token field through to the generated manifests was already covered by existing tests, and the new functionality follows the same code path. Adding separate tests for username/password felt somewhat redundant, but I'm happy to add them if you think they would be valuable. 🙂
Additional Information
Checklist
Tradeoff
None that i can think of.
Potential improvement
First of all, it's absolutely possible that I misunderstood part of the code. I only looked at a small portion of the codebase while investigating this issue, so apologies if that's the case. 🙂
I think it could be worth separating the Helm Autodiscovery auth section/handling for Helm registries and container registries a bit more. The current implementation uses docker.InlineKeyChain for both container registries and Helm registries. That makes sense for OCI registries, but it feels a little less intuitive for non-OCI Helm registries (see the Helm autodiscovery auth handling, which is used for both container manifests and Helm dependency manifests).
I can also imagine situations where different credentials are needed for container images and Helm charts. One example is GitLab, which provides both a Container Registry and a Package Registry on the same domain. They support fine-grained credentials, so you might want separate credentials with access to only one of those registries.
Additionally, the type itself appears to be primarily intended for OCI registries (see the InlineKeyChain type documentation). At the same time, it seems to be used for non-OCI registries as well.
I don't think this is a major issue - the InlineKeyChain type works well as a catch-all since it supports username, password, and token. It just felt a little confusing while I was tracking down this bug.