fix: add username/password auth to helm/argocd autodiscovery - #9706
Conversation
|
Thank you for the pullrequets and the nice words, I'll try to review over the weekend |
|
@itsteddyyo Could you fix the linting issue? |
|
@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 |
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/passwordauth fields in addition totoken. - Extend ArgoCD manifest generation to include
username/passwordauth fields in addition totoken. - 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.
Yes I agree, it's a good observation. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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/passwordauthentication in the auth section. However, it looks like onlytokenis 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-tokeninstead of<registry>.token: my-tokenas 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
tokenfield through to the generated manifests was already covered by existing tests, and the new functionality follows the same code path. Adding separate tests forusername/passwordfelt somewhat redundant, but I'm happy to add them if you think they would be valuable. 🙂Additional Information
Checklist
I have updated the documentation via pull request in website repository.Not applicableTradeoff
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.InlineKeyChainfor 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
InlineKeyChaintype 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
InlineKeyChaintype works well as a catch-all since it supportsusername,password, andtoken. It just felt a little confusing while I was tracking down this bug.