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

add jcasc auto reload sidecar resources #934

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

charliehanaceksonos
Copy link

@charliehanaceksonos charliehanaceksonos commented Oct 19, 2023

What does this PR do?

This PR updates the values summary documentation to include a reference to how to set resource requests and limits for the configAutoReload. Exposing the resource limits and requests for the sidecar allows them to be set to the same value, which allows the whole Pod to be granted a "Guaranteed" QoS class. The helmchart already supports this, though it wasn't reflected in the docs already.

If you modified files in the ./charts/jenkins/ directory, please also include the following:

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. I bumped the "version" key in ./charts/jenkins/Chart.yml.
    Options
  2. I added a new changelog entry to ./charts/jenkins/CHANGELOG.md.
    Options
  3. I followed the technical requirements.
    Options

Special notes for your reviewer

@charliehanaceksonos charliehanaceksonos requested a review from a team as a code owner October 19, 2023 00:13
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure if this is required, from what I understand it's VALUES_SUMMARY.md not VALUES.md

this means some will be omitted especially less interesting ones, all values should be in values.yaml, and commented out if not set, see

resources: {}
# limits:
# cpu: 100m
# memory: 100Mi
# requests:
# cpu: 50m
# memory: 50Mi

@charliehanaceksonos
Copy link
Author

I was only trying to make the documentation consistent, since the other containers specified in values.yaml had references to their associated resources in VALUES_SUMMARY.md. I'll close the PR if you're not going to accept someone offering to improve the docs.

@charliehanaceksonos
Copy link
Author

Would you accept a change adding matching resource requests and limits for the auto config reload container?

@timja
Copy link
Member

timja commented Oct 22, 2023

I was only trying to make the documentation consistent, since the other containers specified in values.yaml had references to their associated resources in VALUES_SUMMARY.md. I'll close the PR if you're not going to accept someone offering to improve the docs.

I see, happy to take the PR, would you be able to resolve the conflicts please?

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.

None yet

2 participants