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

Propagate gomega.Gomega. #3347

Merged
merged 5 commits into from
Oct 29, 2024
Merged

Propagate gomega.Gomega. #3347

merged 5 commits into from
Oct 29, 2024

Conversation

mbobrovskyi
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Propagate gomega.Gomega.

Which issue(s) this PR fixes:

Fixes #3300

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 28, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 28, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 9cc1052
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/672112ca339d6c000851d417

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

/cc @mimowo

@k8s-ci-robot k8s-ci-robot requested a review from mimowo October 28, 2024 18:04
mimowo
mimowo reviewed Oct 29, 2024
View reviewed changes
test/util/util.go
@@ -680,12 +662,22 @@ func ExpectPodsFinalized(ctx context.Context, k8sClient client.Client, keys ...t
for _, key := range keys {
createdPod := &corev1.Pod{}
gomega.EventuallyWithOffset(1, func(g gomega.Gomega) {
g.Expect(k8sClient.Get(ctx, key, createdPod)).To(gomega.Succeed())
g.Expect(client.IgnoreNotFound(k8sClient.Get(ctx, key, createdPod))).To(gomega.Succeed())
Copy link
Contributor

@mimowo mimowo Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why changing this? This is changing the semantics of the assert, maybe we want to check the pod still exists. Should we have 2 functions: ExpectPodsGoneOrFinalized (deleted or no finalizers) and ExpectPodsFinalized (no finalizers). Maybe both functions make sense depending on the context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the pod is finalized, then we can delete it. I think this might have been missed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, but actually as long as the owner workload exists the pod is expected not to be deleted, even if it already does not have the finalizers. So, asserting the set of finalizers (without checking if the pod is gone) seems to make sense.

Copy link
Contributor

@mimowo mimowo Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, no, the relationship is another way round - pod owns the workload. In that case indeed it might be GC-ed. So, better to check ExpectPodsFinalizedOrGone

mimowo
mimowo reviewed Oct 29, 2024
View reviewed changes
test/util/util.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2024
@mbobrovskyi
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2024
@mbobrovskyi
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2024
mimowo
mimowo reviewed Oct 29, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6603d827dd1bed03f94f11e6849fcf8de3cbbac9

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2024
@k8s-ci-robot k8s-ci-robot requested a review from mimowo October 29, 2024 16:52
@mimowo
Copy link
Contributor

mimowo commented Oct 29, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1e0f933bcf97013e78f2e95907ba4bc86d8b7534

@k8s-ci-robot k8s-ci-robot merged commit 8c5d28e into kubernetes-sigs:main Oct 29, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Oct 29, 2024
PBundyra pushed a commit to PBundyra/kueue that referenced this pull request Nov 5, 2024
@mbobrovskyi @PBundyra
Propagate gomega.Gomega. (kubernetes-sigs#3347)
94d9efe 
* Propagate gomega.Gomega.

* Renamed ExpectPodsFinalized to ExpectPodsFinalizedOrGone.

* Fix "should allow to schedule a group of diverse pods" test.

* Add ExpectPodsJustFinalized.

* Fixed flaky "should allow to schedule a group of diverse pods" test.
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@mbobrovskyi @kannon92
Propagate gomega.Gomega. (kubernetes-sigs#3347)
bb2cd3c 
* Propagate gomega.Gomega.

* Renamed ExpectPodsFinalized to ExpectPodsFinalizedOrGone.

* Fix "should allow to schedule a group of diverse pods" test.

* Add ExpectPodsJustFinalized.

* Fixed flaky "should allow to schedule a group of diverse pods" test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y Awaiting requested review from tenzen-y

@trasc trasc Awaiting requested review from trasc

@mimowo mimowo Awaiting requested review from mimowo

Assignees

@mimowo mimowo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9
Development

Successfully merging this pull request may close these issues.

Ensure the err is surfaced in e2e/integration tests to improve debuggability
3 participants
@mbobrovskyi @k8s-ci-robot @mimowo