Serialization support for FlavorResource, FlavorResourceSet and FlavorResourceQuantities #4420
Conversation
The wrapper makes logger calls with sets.Set[resources.FlavorResource] serializable, without changing sets.Set upstream.
k8s-ci-robot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nasedil 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 |
|
Welcome @nasedil! |
k8s-ci-robot
added
cncf-cla: no
|
Hi @nasedil. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
cncf-cla: yes
| frsNeedPreemption sets.Set[resources.FlavorResource] | ||
| frsNeedPreemption FlavorResourceSet |
There was a problem hiding this comment.
I do not think we should do for logging. Instead of that, why we can not just use slice?
frsNeedPreemption.UnsortedList()@mimowo Did you indicate JsonMarshal proposed in this PR? If yes, what is the reason not to use slice?
There was a problem hiding this comment.
Actually, my suggestion was to use slice rather than Json, for sure. I was just thinking about exposing this via String function, but frsNeedPreemption.UnsortedList() is also neat. I'm ok either way.
| @@ -27,4 +29,20 @@ type FlavorResource struct { | |||
| Resource corev1.ResourceName | |||
| } | |||
|
|
|||
| func (fr FlavorResource) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
I think implementing String should be enough, and much simpler. Then, you can convert sets to slices with built-in: sets.List(set)
| @@ -63,14 +64,35 @@ type Preemptor struct { | |||
| applyPreemption func(ctx context.Context, w *kueue.Workload, reason, message string) error | |||
| } | |||
|
|
|||
| type FlavorResourceSet struct { | |||
There was a problem hiding this comment.
why not just type FlavorResourceSet sets.Set[resources.FlavorResource]
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
What type of PR is this?
What this PR does / why we need it:
Properly serializes FlavorResource, FlavorResourceSet and FlavorResourceQuantities types which are logged in integration tests.
Which issue(s) this PR fixes:
Fixes #4137
Special notes for your reviewer:
As discussed in #4137 part of this issue depends on making serializable
k8s.io/apimachinery/pkg/util/sets.Setin upstream repo. In the second commit I createdFlavorResourceSettype to make it work without changing upstream code. Possible options are to keep it temporarily until upstream is fixed, keep it permanently, or just discart and keep only the first commit and wait till upstream issue is fixed.Does this PR introduce a user-facing change?