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.

Sign up for GitHub

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

Jump to bottom

Adding utils.format_quantity #2216

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adding utils.format_quantity #2216

wants to merge 3 commits into from

Conversation

rkschamer
Copy link

@rkschamer rkschamer commented Apr 4, 2024 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

PR adds a utils.format_quantity, which converts a decimal number into a canonical kubernetes quantity.

There is already utils.parse_quantity which I used in my project, but I also needed a way to convert a Decimal number back into a Kubernetes quantity. This PR proposes to add this functionality to the Python K8s client.

Which issue(s) this PR fixes:

Fixes #2205

Special notes for your reviewer:

I couldn't find any tests for the utils modules, neither the parse_quantity function. I'd be very happy to add some tests for format_quantity, but so far I just didn't find the right place for them. Please let me know where I should put them.

Does this PR introduce a user-facing change?

Adding `utils.format_quantity` to convert decimal numbers into a canonical Kubernetes quantity.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

I think that's not applicable for this change - I'm not sure though.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 4, 2024
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Apr 4, 2024 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 4, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @rkschamer!

It looks like this is your first PR to kubernetes-client/python 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 4, 2024
@rkschamer
Copy link
Author

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 4, 2024
@rkschamer rkschamer mentioned this pull request Apr 4, 2024
Open
roycaihw
roycaihw reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

I couldn't find any tests for the utils modules, neither the parse_quantity function. I'd be very happy to add some tests for format_quantity, but so far I just didn't find the right place for them. Please let me know where I should put them.

You could use https://github.com/kubernetes-client/python/blob/master/kubernetes/e2e_test/test_utils.py

@roycaihw roycaihw self-assigned this Apr 4, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 5, 2024
@rkschamer rkschamer marked this pull request as ready for review April 5, 2024 12:44
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2024
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rkschamer
Once this PR has been reviewed and has the lgtm label, please ask for approval from roycaihw. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 5, 2024
@rkschamer
Copy link
Author

I couldn't find any tests for the utils modules, neither the parse_quantity function. I'd be very happy to add some tests for format_quantity, but so far I just didn't find the right place for them. Please let me know where I should put them.

You could use https://github.com/kubernetes-client/python/blob/master/kubernetes/e2e_test/test_utils.py

Perfect, thank you. I've now added the tests for format_quantity and also took the liberty to add a test for parse_quantity. Since pytest haven't been used in test_utils.py I refrained from using @pytest.mark.parametrize. Please let me know if I should change this.

With the tests, I now consider my change as ready for review and hence, I've also changed to an actual PR.

@rkschamer
Copy link
Author

Hi @roycaihw,

I don't want to be pushy, but do you think, you can have a look on the PR?

Thank you,
René

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yliaog yliaog Awaiting requested review from yliaog

@roycaihw roycaihw Awaiting requested review from roycaihw

Assignees

@roycaihw roycaihw

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

format_quantity function missing to convert Decimal values in to K8s quantity string
3 participants
@rkschamer @k8s-ci-robot @roycaihw