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

Adding request extra to support value like "nvidia.com/gpu: 1" #900

Merged
merged 8 commits into from
Mar 12, 2024

Conversation

pjeon2
Copy link
Contributor

@pjeon2 pjeon2 commented Mar 8, 2024

Current helm chart only supports cpu and memory, so I would like to add extra field
For example, I need to add below values

            requests:
              cpu: 10m
              memory: 16Mi
              nvidia.com/gpu: "1"

To support the above values, I added resources.extra field

Closes #901.

Copy link
Owner

@stevehipwell stevehipwell left a 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 @pjeon2. I've added a couple of generic review comments which would need addressing before this PR could be tested.

However that said I'm not sure if the explicit capacity.resources.cpu & capacity.resources.memory are required as capacity.resources could be untyped and directly used with toYaml. What do you think?

@pjeon2
Copy link
Contributor Author

pjeon2 commented Mar 11, 2024

Thanks for the PR @pjeon2. I've added a couple of generic review comments which would need addressing before this PR could be tested.

However that said I'm not sure if the explicit capacity.resources.cpu & capacity.resources.memory are required as capacity.resources could be untyped and directly used with toYaml. What do you think?

Yeah, I think converting entire capacity.resources to toYaml is better approach.
I will apply it into my PR

@pjeon2
Copy link
Contributor Author

pjeon2 commented Mar 11, 2024

@stevehipwell I resolved all your comments.
Would you review it again?

Copy link
Owner

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

I've suggested an update to the value comment for consistency; you'll need to re-run helm-docs after changing this.

Could you also update the changelog under the unreleased section.

@pjeon2
Copy link
Contributor Author

pjeon2 commented Mar 11, 2024

I've suggested an update to the value comment for consistency; you'll need to re-run helm-docs after changing this.

Could you also update the changelog under the unreleased section.

Added changelog as recommended

Copy link
Owner

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehipwell stevehipwell merged commit c1f4c5a into stevehipwell:main Mar 12, 2024
1 check passed
@stevehipwell
Copy link
Owner

@pjeon2 your changes should be available in 0.4.0

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.

Would you add resources.extra field so that I can add my gpu value into the resource?
2 participants