Skip to content
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

[ENHANCE] #639 , Add resources to the Reloader deployment. #692

Merged
merged 7 commits into from
Jun 21, 2024

Conversation

kunj-bosamia
Copy link
Contributor

Memory -> request & limits
Cpu -> requests & limits
are added to the reloader deployment.

Copy link

github-actions bot commented Jun 15, 2024

@bnallapeta Image is available for testing. docker pull ghcr.io/stakater/docs/reloader:v1.0.110

@kunj-bosamia kunj-bosamia changed the title #639 , Add resources to the Reloader deployment. [ENHANCE] #639 , Add resources to the Reloader deployment. Jun 15, 2024
@kunj-bosamia
Copy link
Contributor Author

Related issue -> #639

@MuneebAijaz
Copy link
Contributor

hi @kunj-bosamia thanks for the PR, i'd prefer if the resources are added to plain manifests only and to not change the chart values.
There's a helm template command in workflows, you can add this change in that command instead. https://github.com/stakater/Reloader/blob/master/.github/workflows/push.yaml#L212

Copy link

@kunj-bosamia Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-85dfee8d\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-UBI-85dfee8d

@bnallapeta
Copy link

@kunj-bosamia
While it's a good practice to set resource requests and limits, I believe it's better to provide documentation on how to set these values instead of enforcing them. The default values may not be suitable for every user's environment. For example, clusters with 1000 deployments might find the set limits inadequate, and vice versa. Providing clear guidance allows users to configure their resources based on their specific needs and cluster capacity, balancing best practices with flexibility.

Additionally, our current documentation includes parameters that users can pass during the Helm install command here. They can also set it in values.yaml, where recommended values are commented out.

What do you think?

@kunj-bosamia
Copy link
Contributor Author

Yeah I understand your point of view , the people who are doing helm install to install the reloader app in their k8s environment will have a control to edit the resources.

I think most of the users directly do a

kubectl apply -f https://raw.githubusercontent.com/stakater/Reloader/master/deployments/kubernetes/reloader.yaml

this is what I did in my k8s env , I know it's bad practice but we can get away with it.

My point being at least this relaoser.yaml file should have resources defined in it.
I will make changes in my PR to include resources in just reloader.yaml and not the helm chart.

Copy link

@kunj-bosamia Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-f167a9ba\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-UBI-f167a9ba

@bnallapeta
Copy link

@kunj-bosamia Sure. It makes sense to set moderate requests and limits as a safety net.

Also, could you please add this in docs under this section? It'll help users know they can tweak it as needed.

@kunj-bosamia
Copy link
Contributor Author

@kunj-bosamia Sure. It makes sense to set moderate requests and limits as a safety net.

Also, could you please add this in docs under this section? It'll help users know they can tweak it as needed.

Added 👍

Copy link

@kunj-bosamia Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-d1abc2ee\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-UBI-d1abc2ee

Copy link

@kunj-bosamia Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-0db4652d\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-UBI-0db4652d

Copy link

@kunj-bosamia Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-953957f1\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-692-UBI-953957f1

@bnallapeta bnallapeta merged commit 2e68364 into stakater:master Jun 21, 2024
5 checks passed
@kunj-bosamia kunj-bosamia deleted the issue-639 branch June 21, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants