-
Notifications
You must be signed in to change notification settings - Fork 219
feat: add OCI support for Argo CD sources #4354
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
base: main
Are you sure you want to change the base?
feat: add OCI support for Argo CD sources #4354
Conversation
✅ Deploy Preview for zarf-docs canceled.
|
brandtkeller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first pass - spent a healthy amount of time educating myself on argo further.
Left a nit that I think we can cleanup in a few places. Separately want to consider some additional scope for this change:
- E2E testing - unit tests get us some good feedback loops on the internal dependency and behaviors but this is largely an external integration. I would love to think about how we test this is operational.
- Documentation - right now this can be appending to the existing tutorial. There may be more opportunity for a more granular pass at argocd orchestration docs.
Two minor requests:
- Can you comment on an issue if/when you pick it up for work (or at least with the PR) - In doing so we can assign you and help prevent any colliding submissions (as well as remember to track for reviews on the project board.
- Add context to the PR description. It took me a fair bit of reading to tell that the associated issue was asking for a feature around oci registries when you actually solved for oci images. The former isn't actually as valid and you solved the problem correctly here IMO - but I had to hunt down the delta and process.
Thank you for the continued support and contributions - I very much appreciate it!
| patches = append(patches, operations.ReplacePatchOperation("/data/username", base64.StdEncoding.EncodeToString([]byte(gitServer.PullUsername)))) | ||
| patches = append(patches, operations.ReplacePatchOperation("/data/password", base64.StdEncoding.EncodeToString([]byte(gitServer.PullPassword)))) | ||
|
|
||
| if isOCI { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for code patterns used across zarf (of those replaced thus far) we have been upgrading to use the happy path is left-aligned concept whereby - when possible we process conditions and return early and avoid else returns.
Signed-off-by: Robin Lieb <[email protected]>
96276a0 to
ab7df2b
Compare
Signed-off-by: Robin Lieb <[email protected]>
ab7df2b to
9007e84
Compare
|
@brandtkeller thanks for the initial review. I added changes which addresses all of the mentioned points. For the other two points, will definitely keep that in mind for future work. Update: For the sake of proper documentation, should I create a new issue specifying the OCI support for manifests? |
|
The issue we might have with this is that ArgoCD will try to hit the registry over HTTPS. We were getting this in the Argo logs: Helm allows you to use http via: However it doesn't look like Argo exposes an option for that. It exposes a We're currently trying to work around this by putting an internal The easier option would be to have a HTTPS endpoint exposed on the zarf registry (just internally as a service not as an ingress/GatewayAPI route), even if it just uses a self-signed certificate. Not sure if that's a route we want to take though. |
Description
This PR adds the capability to mutate OCI references in Argo CD resources and replaces the given OCI reference to the Zarf registry URL.
This change adds the possibility to use Manifest and Helm Charts in the repoURL in the Application, like explained in the OCI Docs in Argo CD with the first two examples. To archive that it also changes the patching of the URL in repository secrets to use the registry instead of the git server if oci is used.
Related Issue
Relates to #3046
Checklist before merging