-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor: drop GetPodByVirtualMachineInstance,getPodsByLabel . Use libpod.GetPodByVirtualMachineInstance
instead at all its callers.
#11474
Conversation
Hi @TheRealSibasishBehera. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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/test-infra repository. |
2bb3991
to
b76a165
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.
The original issue this PR treats needs to be changed.
Please drop GetPodByVirtualMachineInstance
from utils.go
and replace it in all callers to libpod.GetPodByVirtualMachineInstance
that already exists.
b76a165
to
620402b
Compare
tests/utils.go
Outdated
|
||
return &pods.Items[0] | ||
} | ||
|
||
func getPodsByLabel(label, labelType, namespace string) (*k8sv1.PodList, error) { |
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.
Is it used now?
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.
no it's unused now
should I remove this as well?
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.
Yes
/test pull-kubevirt-build |
/test all |
/sig code-quality |
620402b
to
c8f6eff
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.
Thank you.
Last thing:
- The commit message line is too long, consider:
refactor: drop GetPodByVirtualMachineInstance,getPodsByLabel
Use `libpod.GetPodByVirtualMachineInstance` instead at all its callers.
- PR subject: Please use the same as the commit.
Other than that, LGTM.
Use libpod.GetPodByVirtualMachineInstance instead at all its callers. Signed-off-by: TheRealSibasishBehera <fangedhamster3114@gmail.com>
c8f6eff
to
838aea4
Compare
libpod.GetPodByVirtualMachineInstance
instead at all its callers.
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.
/test all
@TheRealSibasishBehera: The following test failed, say
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. |
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.
Thanks, looks good!
@EdDev Thanks for taking care of my mixup.
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enp0s3 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 |
Required labels detected, running phase 2 presubmits: |
/retest-required |
1 similar comment
/retest-required |
What this PR does
Before this PR:
The function
GetPodByVirtualMachineInstance
intests/utils.go
was used to get pods using labels.After this PR:
GetPodByVirtualMachineInstance
andgetPodsByLabel
fromutils.go
.libpod.GetPodByVirtualMachineInstance
for its callersFixes #11440
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note