-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add overlay for deployment #20
Add overlay for deployment #20
Conversation
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.
Thanks for the PR @miles-w-3 I've left a super minor nit for now. This seems fine to me but I'm at a conf for the next few days and would prefer to take a closer look when I return. I won't leave it hanging for too long though. Cheers.
4530a8c
to
3e25ad0
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.
I'm not that familiar with the use case here but I think these changes seem fine. Thanks for the PR @miles-w-3
fwiw, the use case is if you need to override Kubernetes specs like the memory limit, image tag, environment variables, etc. Under the current system with the operator SDK, it was designed that you change/add those fields with kustomize, but it's not really compatible with the helm packaging solution. As I remember, there were a lot of issues back when this image was in the main repo that were related to "why can't I change these deployment fields with the helm chart" |
3e25ad0
to
4f30060
Compare
Added commit containing documentation and a check for the |
Is this waiting for something specific? |
Closed and re-opened to kick off ci with the changes from #25 @ChrisLee-JackHenry sorry I missed the comment and didn't respond earlier. this has been on hold until we fixed ci. |
@miles-w-3 Hey, looks like there's a type error in here:
|
thanks for the review, will rebase and address these fixes. |
4f30060
to
ba7e86b
Compare
@oraNod addressed the error and standardized the behavior to merge at the root of the doc as I had initially written in the readme |
ba7e86b
to
f6ae5cd
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.
Thanks @miles-w-3
The configuration of the `awx-operator-controller-manager` `Deployment` resource can be overridden by the | ||
`operator-controller` field. Any fields specified under this key will map directly onto the root hierarchy of the [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/) configuration. | ||
|
||
For example, to override the replicas of the controller deployment, use: |
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.
While the example of the replica is valid. This is not really a common scenario to customize the operator replica (maybe to scale up/scale down). One more common use case is to specify a different container image for each container as seen in this issue or that one to cover disconnected/restricted environments.
Another useful example would be to provide an example for setting up resource requests and limits, this would cover this open issue.
What do you think? Could you maybe provide an example on how to do that @miles-w-3
Thanks for the PR btw!
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.
@schen1 Perhaps we should capture these items in a follow up to improve the docs. While I think extensive documentation is always a good thing, I'm not sure we should hold up this PR much longer. Any thoughts @miles-w-3 ?
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.
fine for me :)
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 am fine either way. The use case for overriding an image tag is tricky because of the way lists are merged in helm.
Lists are overridden instead of merged for each layer. So under this proposal, if you want to change the image version, you have to override the entire container list spec. For that reason, this solution is more optimal for adding things like annotations or service account specs to the controller's pods.
If there is desire for more targeted overrides of specific containers, that could be another feature for a template that iterates through a list, matches a container name, and then merges a set override
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.
Thanks for the details and the great contribution. Then it makes sense to have it as a different feature/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.
Looks good. Thanks for the work on this @miles-w-3
Closes #14. Deployment resource is now fully templatable via .Values.controller.spec, .labels, and .annotations.
Only regression I see is that the previously spliced-in template for .Values.Operator.replicas will not work, but I believe this setup is more scalable