-
Notifications
You must be signed in to change notification settings - Fork 308
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
Move Pod labels and annotation to separate package. #4472
Move Pod labels and annotation to separate package. #4472
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
897a1e2
to
d0d2c0d
Compare
/cc @PBundyra |
d0d2c0d
to
63b67ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we put these constants in pkg/controller/jobs/pod
, there are cycle imports, right?
If yes, could you point out the place for cycle imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the labels and annotations in pkg/util/testingjobs/pod/wrappers.go
to use constants instead:
kueue/pkg/util/testingjobs/pod/wrappers.go
Lines 158 to 170 in d677d9e
func (p *PodWrapper) PodGroupServingAnnotation(enabled bool) *PodWrapper { | |
return p.Annotation("kueue.x-k8s.io/pod-group-serving", strconv.FormatBool(enabled)) | |
} | |
// RoleHash updates the pod.RoleHashAnnotation of the pod | |
func (p *PodWrapper) RoleHash(h string) *PodWrapper { | |
return p.Annotation("kueue.x-k8s.io/role-hash", h) | |
} | |
// KueueSchedulingGate adds kueue scheduling gate to the Pod | |
func (p *PodWrapper) KueueSchedulingGate() *PodWrapper { | |
return p.Gate("kueue.x-k8s.io/admission") | |
} |
However, this introduced circular dependencies. To resolve this, I moved the code into a separate package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the cause for these manifest changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change +kubebuilder markers. I think it happens after moving constants to separate package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, the changes only involve reordering the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Anyway, it seems not to change manifests except for orders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
/lgtm
/approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Anyway, it seems not to change manifests except for orders.
LGTM label has been added. Git tree hash: 48d4a423ce61a43877767afb270531158aa6317c
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbobrovskyi, tenzen-y The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Move Pod labels and annotation to separate package.
Which issue(s) this PR fixes:
Part of #3653
Special notes for your reviewer:
Does this PR introduce a user-facing change?