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 function to parse quantity #37

Merged
merged 13 commits into from
Jul 8, 2022
Merged

Conversation

sed-i
Copy link
Contributor

@sed-i sed-i commented Jul 4, 2022

This PR add a utility function for parsing K8s "quantity" from string.
The value is parsed into an absolute (unitless) decimal.Decimal instance, and not into the K8s "canonical representation". This way the code is simpler and values are readily comparable.

References:

  1. https://github.com/kubernetes-client/python/blob/master/kubernetes/utils/quantity.py
  2. https://github.com/kubernetes/apimachinery/blob/master/pkg/api/resource/quantity.go

Fixes #36.

Copy link
Owner

@gtsystem gtsystem left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your PR. Can you add this function to the documentation together with an example? If you introduce the utils sub-package you can create docs/utils.md

lightkube/core/quantity.py Outdated Show resolved Hide resolved
lightkube/core/quantity.py Outdated Show resolved Hide resolved
lightkube/core/quantity.py Outdated Show resolved Hide resolved
lightkube/core/quantity.py Outdated Show resolved Hide resolved
lightkube/core/quantity.py Outdated Show resolved Hide resolved
@sed-i
Copy link
Contributor Author

sed-i commented Jul 6, 2022

Thanks @gtsystem! Ready for re-review.

@sed-i sed-i requested a review from gtsystem July 6, 2022 18:55
lightkube/utils/quantity.py Outdated Show resolved Hide resolved
lightkube/utils/quantity.py Outdated Show resolved Hide resolved


@overload
def equals_canonically(first: Optional[dict], second: Optional[dict]) -> bool:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see sufficient benefits of supporting dicts. Users can always convert dicts to ResourceRequirements (ResourceRequirements.from_dict). I'll suggest to drop this for now, so that we can have a simpler implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dict here is not an alternative representation of
ResourceRequirements, but of ResourceRequirements.limits and ResourceRequirements.requests, i.e. one level down.

The tests demonstrate this. With the overload we're able to do both:

equals_canonically({"cpu": "0.6"}, {"cpu": "600m"})

and

equals_canonically(
    ResourceRequirements(limits={"cpu": "0.6"}), 
    ResourceRequirements(limits={"cpu": "600m"})
)

If feasible, we could change ResourceRequirements to use TypedDict

+class ResourceSpecDict(TypedDict, total=False):
+    cpu: Optional[str]
+    memory: Optional[str]

@dataclass
class ResourceRequirements(DataclassDictMixIn):
-    limits: 'dict' = None
-    requests: 'dict' = None
+    limits: ResourceSpecDict = None
+    requests: ResourceSpecDict = None

In which case the overload would become

@overload
def equals_canonically(first: Optional[ResourceSpecDict], second: Optional[ResourceSpecDict]) -> bool:

Copy link
Owner

Choose a reason for hiding this comment

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

Got it, I was confused by the function description "Compare two resource requirements for numerical equality." I assumed that the only acceptable type is ResourceRequirements or the equivalent dict. Maybe we can clarify further?

No need to introduce the TypedDict as anyway all the keys are optional and we can have further keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit.

lightkube/utils/quantity.py Outdated Show resolved Hide resolved
lightkube/utils/quantity.py Outdated Show resolved Hide resolved
lightkube/utils/quantity.py Show resolved Hide resolved
@sed-i sed-i requested a review from gtsystem July 7, 2022 14:04
@sed-i sed-i requested a review from gtsystem July 7, 2022 18:56
TypeError: Subscripted generics cannot be used with class and instance checks
@sed-i sed-i requested a review from gtsystem July 8, 2022 09:23
@gtsystem gtsystem merged commit c558466 into gtsystem:master Jul 8, 2022
@gtsystem
Copy link
Owner

gtsystem commented Jul 8, 2022

Approved and merged. We have another contribution in progress, once that is completed I will create a public release. Thanks again.

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.

Add means to compare quantities
2 participants