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

test/utils: Remove NotDeleted() and NotDeletedVMs() #11850

Merged
merged 4 commits into from
May 7, 2024

Conversation

assafad
Copy link
Contributor

@assafad assafad commented May 5, 2024

What this PR does

  • Move NotDeleted() to /libreplicaset (since it's used only by replicaset test), rename it to FilterNotDeletedVMIs() and remove its named returned value.
  • Move NotDeletedVMs() to pool_test.go (the only file it's used in), and improve by integrating it within
    notDeletedVMs() function, which was renamed to filterNotDeletedVMIsOwnedByPool().

/sig code-quality

None

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. sig/code-quality dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels May 5, 2024
@kubevirt-bot kubevirt-bot added the sig/buildsystem Denotes an issue or PR that relates to changes in the build system. label May 5, 2024
Copy link
Member

@orelmisan orelmisan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @assafad.

AFAIU, NotDeleted() and NotDeletedVMs() are unrelated.
Could you please split the change to two commits? (one for each function move)

@orelmisan
Copy link
Member

Please consider renaming NotDeleted to FilterVMIsNotMarkedForDeletion or something along those lines, in order to better understand the action it performs from its name.

Same for NotDeletedVMs.

Please also consider removing the named return values from their signatures.

@assafad assafad changed the title test, utils: Move NotDeleted() and NotDeletedVMs() test, utils: Remove NotDeleted() and NotDeletedVMs() May 5, 2024
@assafad
Copy link
Contributor Author

assafad commented May 5, 2024

orelmisan updated.

Move NotDeleted() to libreplicaset, as it's used by replicaset test
only.

Signed-off-by: assafad <aadmi@redhat.com>
Rename NotDeleted() to FilterNotDeletedVMIs() and remove its named
returned value

Signed-off-by: assafad <aadmi@redhat.com>
Move NotDeletedVMs() to pool_test.go as it's the only place it's used
in.

Signed-off-by: assafad <aadmi@redhat.com>
@assafad assafad changed the title test, utils: Remove NotDeleted() and NotDeletedVMs() test/utils: Remove NotDeleted() and NotDeletedVMs() May 5, 2024
tests/pool_test.go Outdated Show resolved Hide resolved
tests/pool_test.go Outdated Show resolved Hide resolved
Improve by removing named returned value and integrate both
NotDeletedVMs() functions.

Signed-off-by: assafad <aadmi@redhat.com>
@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented May 5, 2024

@assafad: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-kind-1.27-vgpu 87d8cc5 link false /test pull-kubevirt-e2e-kind-1.27-vgpu

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/test-infra repository. I understand the commands that are listed here.

Copy link
Member

@orelmisan orelmisan left a comment

Choose a reason for hiding this comment

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

Thank you @assafad

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2024
tests/libreplicaset/replicaset.go Outdated Show resolved Hide resolved
tests/libreplicaset/replicaset.go Show resolved Hide resolved
tests/pool_test.go Outdated Show resolved Hide resolved
tests/pool_test.go Show resolved Hide resolved
@assafad
Copy link
Contributor Author

assafad commented May 6, 2024

@xpivarc Hi, the PR started with 1 commit, then 2 commits, and finally, per @orelmisan's request, I split it to 4 commits.

@xpivarc
Copy link
Member

xpivarc commented May 6, 2024

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@kubevirt-bot kubevirt-bot merged commit fba201f into kubevirt:main May 7, 2024
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/code-quality size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants