-
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
support app set #266
support app set #266
Conversation
README documentation should clearly mention that a package can include |
e02b8e5
to
2bb1618
Compare
Signed-off-by: Manabu McCloskey <[email protected]>
Signed-off-by: Manabu McCloskey <[email protected]>
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.
I think this looks good. I think there's an opportunity to clean up the code a bit if possible
return ctrl.Result{}, fmt.Errorf("getting argocd application set object: %w", err) | ||
} | ||
|
||
foundAppSetObj.Spec = appSet.Spec |
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.
the reconcilliation logic is duplicated here with the above case. Is it possible to dedupe?
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.
I considered it but I don't think we'd save any lines of code or performance because we'd have to type cast in the new function then branch out anyways. I figured a bit of duplication here is acceptable.
@@ -38,7 +44,7 @@ func TestReconcileCustomPkg(t *testing.T) { | |||
ErrorIfCRDPathMissing: true, | |||
Scheme: s, | |||
BinaryAssetsDirectory: filepath.Join("..", "..", "..", "bin", "k8s", | |||
fmt.Sprintf("1.27.1-%s-%s", runtime.GOOS, runtime.GOARCH)), | |||
fmt.Sprintf("1.29.1-%s-%s", runtime.GOOS, runtime.GOARCH)), |
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.
Do you want to pull this out as a constant? It's used again below
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.
We need to make this more dynamic and define it at a project level. We need this in make file too. Probably better to just pull from a file or something similar. But I think it's out side scope for this PR.
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. thanks for this!
This adds support for application sets.
Example application sets are located in
pkg/controllers/custompackage/test/resources/customPackages/applicationSet/
Need: