Conversation
|
README documentation should clearly mention that a package can include |
e02b8e5 to
2bb1618
Compare
Signed-off-by: Manabu McCloskey <manabu.mccloskey@gmail.com>
Signed-off-by: Manabu McCloskey <manabu.mccloskey@gmail.com>
omrishiv
left a comment
There was a problem hiding this comment.
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.
the reconcilliation logic is duplicated here with the above case. Is it possible to dedupe?
There was a problem hiding this comment.
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.
| 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.
Do you want to pull this out as a constant? It's used again below
There was a problem hiding this comment.
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.
nimakaviani
left a comment
There was a problem hiding this comment.
LGTM. thanks for this!
This adds support for application sets.
Example application sets are located in
pkg/controllers/custompackage/test/resources/customPackages/applicationSet/Need: