-
Notifications
You must be signed in to change notification settings - Fork 62
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
ensure multi source spec is handled #124
Conversation
Signed-off-by: Manabu McCloskey <[email protected]>
- repoURL: cnoe://busybox | ||
targetRevision: HEAD | ||
path: "." |
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.
Sorry if I missed something but this test case only includes one source and not (as title mentions) several sources !
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.
Yeah it's meant to represent a case when the sources field is used. Which is not exactly the same as what is described in the title. I'll update the test.
Signed-off-by: Manabu Mccloskey <[email protected]>
54f86d0
to
29d1e3f
Compare
Updated it to further handle a case where the same directory is specified twice in the spec. This is a valid configuration since you can specify the path parameter to distinguish the two. spec:
sources:
- repoURL: cnoe://app2
targetRevision: HEAD
path: "one"
- repoURL: cnoe://app2
targetRevision: HEAD
path: "two" |
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.
LGTM
@@ -190,8 +190,13 @@ func (r *Reconciler) reconcileGitRepo(ctx context.Context, resource *v1alpha1.Cu | |||
} | |||
return nil | |||
}) | |||
// it's possible for an application to specify the same directory multiple times in the spec. | |||
// if there is a repository already created for this package, no further action is necessary. | |||
if !errors.IsAlreadyExists(err) { |
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.
Good catch
Fixes an issue when you are using multi sources in ArgoCD, the repo url was not updated correctly. It was because we were not updating the correct object.