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 get subnet #2411

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Test get subnet #2411

merged 2 commits into from
Oct 19, 2023

Conversation

majorchork
Copy link
Contributor

What this PR does / why we need it:
this pr adds unit test for getSubnetID LbaasV2 method in loadbalancer.go
Which issue this PR fixes(if applicable):
refers #2400

Special notes for reviewers:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 9, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 9, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @majorchork. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 9, 2023
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

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

Seems like a case testing this codepath is missing?

pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
@dulek
Copy link
Contributor

dulek commented Oct 9, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2023
@majorchork majorchork force-pushed the test-getSubnetID branch 3 times, most recently from 2c2bfca to ade8a08 Compare October 9, 2023 17:39
@majorchork
Copy link
Contributor Author

Seems like a case testing this codepath is missing?

yea! that was the one with the wrong name, I've updated it now

@majorchork majorchork force-pushed the test-getSubnetID branch 3 times, most recently from fa52930 to 492479d Compare October 9, 2023 18:41
@majorchork majorchork requested a review from dulek October 9, 2023 18:42
Copy link
Member

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

Reviewing Test_getSubnetID

pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023
@dulek
Copy link
Contributor

dulek commented Oct 13, 2023

/approve

I'll leave /lgtm for @pierreprinetti, I'm not 100% sure this is what he meant on the error checking front.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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 13, 2023
@majorchork
Copy link
Contributor Author

/approve

I'll leave /lgtm for @pierreprinetti, I'm not 100% sure this is what he meant on the error checking front.

Thanks a lot! Noted

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023
Copy link
Member

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

/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 19, 2023
@pierreprinetti
Copy link
Member

Please rebase to enable the merge 🙏

@majorchork
Copy link
Contributor Author

Please rebase to enable the merge 🙏

I already have seems like the bot forgot to remove the label, I'll just force push again and see if the bot picks it this time
Screenshot 2023-10-19 at 06 43 21

@majorchork
Copy link
Contributor Author

majorchork commented Oct 19, 2023

@k8s-ci-robot /verify needs-rebase

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2023
@pierreprinetti
Copy link
Member

Please rebase to enable the merge 🙏

I already have seems like the bot forgot to remove the label, I'll just force push again and see if the bot picks it this time Screenshot 2023-10-19 at 06 43 21

What do you mean, "Your branch is ahead of 'origin/master' by 17 commits"? This is not expected. Your local master branch is not aligned with origin.

To rebase on origin's master, try:

git checkout test-getSubnetID
git fetch origin
git rebase origin/master

Additionally, if you want to get rid of those 17 commits on your local master branch, here's how to do it:

git checkout master
git fetch origin
git reset --hard origin/master
git clean -fdx

@majorchork
Copy link
Contributor Author

majorchork commented Oct 19, 2023

git reset --hard origin/master
Screenshot 2023-10-19 at 07 13 47

😅 my bad, it's ahead cause I've not published my local to my origin, I always just fetch upstream and merge then checkout to the branch I want to rebase before pushing, I never actually push the changes on my local origin
git fetch upstream (upstream is https://github.com/kubernetes/cloud-provider-openstack/master)
git merge upstream/master
git checkout test-getSubnetId
git rebase master
so my local master is ahead by all the changes I've fetched from the upstream,
I've updated, gotten rid of the commits, and rebased now

@pierreprinetti
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2023
@k8s-ci-robot k8s-ci-robot merged commit 92bde6c into kubernetes:master Oct 19, 2023
6 checks passed
@majorchork majorchork deleted the test-getSubnetID branch October 20, 2023 07:20
mandre pushed a commit to shiftstack/cloud-provider-openstack that referenced this pull request Feb 6, 2024
* test: LbaasV2 method buildPoolCreateOpt

* test: LbaasV2 method getSubnetID
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants