Skip to content

use parse_quantity to convert string to raw resource #522

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

Merged

Conversation

KPostOffice
Copy link
Collaborator

@KPostOffice KPostOffice commented Apr 23, 2024

Issue link

RHOAIENG-5000

What changes have been made

Directly use mem string with units rather than attempting to convert back and forth to int. If the actual number of bytes used by a cluster is needed for some reason in the future, use kubernetes.utils.parse_quantity

Verification steps

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@KPostOffice KPostOffice force-pushed the parseResourceStrings branch 6 times, most recently from cf78cd4 to 1d4237c Compare April 24, 2024 19:28
@KPostOffice KPostOffice force-pushed the parseResourceStrings branch from 1d4237c to 5691a20 Compare April 24, 2024 19:30
Copy link
Contributor

@abhijeet-dhumal abhijeet-dhumal left a comment

Choose a reason for hiding this comment

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

/lgtm
Manually verified, using demo notebook.

Reproducer :
In current upstream version, Using '500m' as a string value in ClusterConfiguration and by running

from codeflare_sdk.cluster import get_cluster
get_cluster(<raycluster_name>,<namespace_name>)

ValueError: invalid literal for int() with base 10: '500m'

This PR resolved above error by accepting string value as input to ClusterConfiguration parameters.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abhijeet-dhumal
Once this PR has been reviewed and has the lgtm label, please ask for approval from kpostoffice. 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

@KPostOffice KPostOffice merged commit 59cbccc into project-codeflare:main Apr 26, 2024
3 of 4 checks passed
Ygnas added a commit to Ygnas/codeflare-sdk that referenced this pull request Apr 30, 2024
KPostOffice added a commit to KPostOffice/codeflare-sdk that referenced this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants