-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow creation of ArgoCDs and AppProjects #151
Conversation
I'll reuse an existing golden-test to reduce the amount of crap in here |
588432f
to
36e0972
Compare
36e0972
to
d75a4ee
Compare
While this component already managed the ArgoCD operator and the Syn ArgoCD instance, additional ArgoCD instances could only be deployed as adhoc-manifests. While this works, it is not possible to leverage Syn features such as hierarchical configuration, resulting in quite some duplication. This commit introduces the new parameters `argocds` and `projects`, which allow the creation of `ArgoCD` and `AppProject` instances respectively. In general, the component tries to be as un-intrusive as possible, so we only set minimal default values: For `ArgoCD`: spec.applicationInstanceLabelKey` - if this value collides for two ArgoCD instances on the same cluster, ArgoCD cannot properly determine which resources belong to "itself", and might try to prune resources managed by other ArgoCD instances. Signed-off-by: Manuel Hutter <manuel@hutter.io>
d75a4ee
to
722fe06
Compare
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 overall. Some inline comments for potential larger improvements, but I have no strong opinion either way.
Thanks for the input @simu, I have updated the PR |
As mentioned in the code review, AppProject resources created usig this component usually only exist within the context of an ArgoCD instance ALSO deployed using this component, so it makes sense to nest these resources. As a side-effect, this allows me to use `instances` as the parameter key, which I prefer :-) Additionally we now validate that no two instances are deployed to the same namespace. Signed-off-by: Manuel Hutter <manuel@hutter.io>
b6143e1
to
c4c54e5
Compare
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 now. The feat:
prefix in the PR title is unnecessary and would be added verbatim to the changelog in the release notes. Setting the enhancement
label is sufficient to mark the change as a new feature/enhancement.
While this component already managed the ArgoCD operator and the Syn ArgoCD instance, additional ArgoCD instances could only be deployed as adhoc-manifests. While this works, it is not possible to leverage Syn features such as hierarchical configuration, resulting in quite some duplication.
This commit introduces the new parameters
argocds
andprojects
, which allow the creation ofArgoCD
andAppProject
instances respectively.In general, the component tries to be as un-intrusive as possible, so we only set minimal default values:
For
ArgoCD
:spec.applicationInstanceLabelKey
- if this value collides for two ArgoCD instances on the same cluster, ArgoCD cannot properly determine which resources belong to "itself", and might try to prune resources managed by other ArgoCD instances.Checklist
changelog.
The PR has a meaningful description that sums up the change. It will be
linked in the changelog.
bug
,enhancement
,documentation
,change
,breaking
,dependency
as they show up in the changelog.