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

[WIP] feat: allow multiple sources for application #9609

Closed
wants to merge 2 commits into from
Closed

[WIP] feat: allow multiple sources for application #9609

wants to merge 2 commits into from

Conversation

ishitasequeira
Copy link
Member

@ishitasequeira ishitasequeira commented Jun 8, 2022

This is just a draft PR to indicate that the work has started on the proposal implementation.
The PR is not ready for review.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@cbrdy
Copy link

cbrdy commented Jun 24, 2022

I have got couple of question on this feature.

  1. can I use multiple application sources when using the config management plugin ?
  2. If 2 application sources are defined lets say and 2nd source has only values files, how do the helm chart from first application source refer the values from the 2nd application source ?

Thanks,
cbrdy

@crenshaw-dev
Copy link
Member

@cbrdy yes, multiple sources will be allowed for all config management tools, including plugins.

References will be possible via path string substitution: https://github.com/argoproj/argo-cd/blob/master/docs/proposals/multiple-sources-for-applications.md#add-optional-ref-field-to-application-source

Substitution support will be added on a field by field basis. As of right now, there are no plans to make path variables substitutable in any field in the plugin source spec. So while plugins will support multiple sources, there are no current plans to make it possible for the multiple sources to reference each other.

@cbrdy
Copy link

cbrdy commented Jun 27, 2022

@cbrdy yes, multiple sources will be allowed for all config management tools, including plugins.

References will be possible via path string substitution: https://github.com/argoproj/argo-cd/blob/master/docs/proposals/multiple-sources-for-applications.md#add-optional-ref-field-to-application-source

Substitution support will be added on a field by field basis. As of right now, there are no plans to make path variables substitutable in any field in the plugin source spec. So while plugins will support multiple sources, there are no current plans to make it possible for the multiple sources to reference each other.

Thank you so much. I’m eagerly waiting for this version. When is this being released?

@crenshaw-dev
Copy link
Member

@cbrdy we're planning on 2.5, in August.

@arloliu
Copy link

arloliu commented Jul 5, 2022

We have a question about this proposal.
When setting multiple sources, the repoURL is not always pointing to git repo. e.g: it points to the helm chart repo which contains helm package index when we want to specify the published helm chart package.

So how do we properly detect the state of all sources? how to detect if the source has changed and needs to sync or invalidate the cache?

@ishitasequeira
Copy link
Member Author

We have a question about this proposal. When setting multiple sources, the repoURL is not always pointing to git repo. e.g: it points to the helm chart repo which contains helm package index when we want to specify the published helm chart package.

So how do we properly detect the state of all sources? how to detect if the source has changed and needs to sync or invalidate the cache?

While checking for changes to the app manifests, we check for changes in each source mentioned in sources field. If we detect the change in even a single source, we update the manifest, sync all the resources and invalidate the cache.

@mrkwtz
Copy link

mrkwtz commented Jul 7, 2022

I've got a question: is that only for multiple Helm sources or could we have source with a Helm chart and another one with plain Kubernetes manifests that get applied additionally?

@ocraviotto
Copy link
Contributor

@ishitasequeira and @crenshaw-dev

Thanks for the initial work gone into this (including the implementation proposal), really looking forward to the end result.
Question(s): is this still being worked on? I see the last commit from over 2 months ago so just wondering if the plan is still to make it to 2.5? I don't see the 2.5 milestone on this PR, though it is in the original issue #2789.

Thanks again for all of your work. If there is anything I could help with happy to. I have not had time to review what's done already as part of this PR, but depending on your answer I'll have a go (if this has now stalled for whatever reason for instance).

@ishitasequeira
Copy link
Member Author

@ocraviotto I am working on this PR and will be having an update to this and ready for review by this weekend.

@ishitasequeira ishitasequeira added this to the v2.5 milestone Aug 17, 2022
@ocraviotto
Copy link
Contributor

@ocraviotto I am working on this PR and will be having an update to this and ready for review by this weekend.

Thanks so much for the quick reply @ishitasequeira - and for the good news :)

@ishitasequeira
Copy link
Member Author

ishitasequeira commented Aug 23, 2022

Getting tons of merge conflicts and different kinds of issues. Raised a new PR for this feature #10432. The new PR would be ready for review soon.

@hutger
Copy link

hutger commented Aug 27, 2022

@cbrdy yes, multiple sources will be allowed for all config management tools, including plugins.

References will be possible via path string substitution: https://github.com/argoproj/argo-cd/blob/master/docs/proposals/multiple-sources-for-applications.md#add-optional-ref-field-to-application-source

Substitution support will be added on a field by field basis. As of right now, there are no plans to make path variables substitutable in any field in the plugin source spec. So while plugins will support multiple sources, there are no current plans to make it possible for the multiple sources to reference each other.

@crenshaw-dev , I have one question related to the new .argocd-source-<app>.yaml file injected by argocd-image-updater plugin. Considering the following code:

spec:
  sources:
    - repoURL: https://github.com/my-org/my-repo  # path is missing so no manifests are generated
      targetRevision: master
      ref: myRepo                                 # repo is available via symlink "myRepo"
    - repoURL: https://github.com/helm/charts
      targetRevision: master
      path: incubator/elasticsearch               # path "incubator/elasticsearch" is used to generate manifests
      helm:
        valueFiles:
          - $myRepo/values.yaml                   # values.yaml is located in source with reference name $myRepo
...

Considering that .argocd-source file will be stored at my-repo, how will be able to inject the source file content as parameter for the helm chart, same we do for valueFiles (which refers to $myRepo/values.yaml)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants