enh: add tests for checkListenerPorts #2405

sakshi-1505 · 2023-10-08T07:20:18Z

What this PR does / why we need it:
It increase unit test coverage for LB file

Which issue this PR fixes(if applicable):
refers #2400

Special notes for reviewers:

Release note:

NONE

k8s-ci-robot · 2023-10-08T07:20:27Z

Hi @sakshi-1505. 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.

jichenjc · 2023-10-09T07:12:43Z

/ok-to-test

sakshi-1505 · 2023-10-09T07:51:35Z

/retest

dulek · 2023-10-09T17:57:32Z

pkg/openstack/loadbalancer_test.go

+						Port:     9090,
+					}: {
+						ID:   "listenerid",
+						Tags: []string{"test-lb", "test-lb2"},


I think we can only have one LB on a listener in our logic.

dulek · 2023-10-09T18:00:27Z

pkg/openstack/loadbalancer_test.go

+	type fields struct {
+		LoadBalancer LoadBalancer
+	}


Doesn't seem you ever use this? Can we remove it?

It was added to mimic the interface of LoadBalancer, although we didn't use any field for the caller but it is actually needed to invoke the function from LoadBalancer interface.

But no test case defines LoadBalancer as anything else than LoadBalancer{}. Can't we just define it in the loop that goes over the test cases?

Yes that makes sense, I will just define it in the loop

dulek · 2023-10-09T18:00:31Z

pkg/openstack/loadbalancer_test.go

+	}
+	tests := []struct {
+		name    string
+		fields  fields


dulek · 2023-10-09T18:01:43Z

pkg/openstack/loadbalancer_test.go

+		{
+			name: "error is not thrown if lbOwner is present & no tags on service",
+			args: args{
+				service: &corev1.Service{
+					Spec: corev1.ServiceSpec{
+						Ports: []corev1.ServicePort{
+							{
+								Name:     "service",
+								Protocol: "https",
+								Port:     9090,
+							},
+						},
+					},
+				},
+				curListenerMapping: map[listenerKey]*listeners.Listener{
+					{
+						Protocol: "https",
+						Port:     9090,
+					}: {
+						ID: "listenerid",
+					},
+				},
+				isLBOwner: true,
+				lbName:    "test-lb",
+			},
+			wantErr: false,
+			fields: fields{
+				LoadBalancer: LoadBalancer{},
+			},
+		},


We should have a case when lbOwner=true and there are some tags on the Service.

dulek · 2023-10-12T16:11:04Z

pkg/openstack/loadbalancer_test.go

+	type fields struct {
+		LoadBalancer LoadBalancer
+	}


But no test case defines LoadBalancer as anything else than LoadBalancer{}. Can't we just define it in the loop that goes over the test cases?

sakshi-1505 · 2023-10-16T14:05:25Z

@pierreprinetti Please check this I have made all the changes which @dulek had requested

pierreprinetti · 2023-10-16T15:44:21Z

Thank you! Please rebase and we're good 🚀

sakshi-1505 · 2023-10-16T17:42:10Z

Thank you! Please rebase and we're good 🚀

I think it's already aligned with master branch, can you please check again?

pierreprinetti · 2023-10-16T18:42:40Z

the label said otherwise . Can you please check? This also needs a ~~rebase~~ squash by the way.

sakshi-1505 · 2023-10-16T19:10:45Z

There are multiple merge commits from main in this branch, squashing would make the git history dirty due to those. Do you know how can I get over it?

…

On Tue, 17 Oct 2023 at 00:12, Pierre Prinetti ***@***.***> wrote: Right: I should have said “squash”, not “rebase” — Reply to this email directly, view it on GitHub <#2405 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BDCZVU3Q5QTBCSZOUF6CJRTX7V52XAVCNFSM6AAAAAA5XPXQTWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRVGA3TQMZWGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pierreprinetti · 2023-10-17T09:29:35Z

The problem here is that you have meged master into your branch, whereas you should have rebased your branch onto master.

You have several options, the cleanest of which would entail git fetch --all && git rebase -i origin/master. I encourage you to explore that.

If you find yourself stuck, be aware that you can cancel the rebase at any time with git rebase --abort.

If instead you just want the job done and you're not afraid of using any means necessary:

git reset origin/master --hard \
	&& git apply <(curl -sSL https://github.com/kubernetes/cloud-provider-openstack/pull/2405.diff) \
	&& git add --all \
	&& git commit

But please don't tell anyone 🤫

In any case, for the next time, I strongly advise you rebase instead of merging:

git fetch origin
git rebase origin/master

sakshi-1505 · 2023-10-17T14:34:56Z

Thanks @pierreprinetti I will remember this from now on, can you please approve this?

pierreprinetti · 2023-10-17T15:49:09Z

Unfortunately, another PR merged and you've won a new rebase round

pierreprinetti · 2023-10-17T15:59:42Z

Or maybe I am misinterpreting the label?
/lgtm
/assign jichenjc

jichenjc · 2023-10-18T01:16:42Z

/approve
/hold

hold for conflict solve, then let's unhold

k8s-ci-robot · 2023-10-18T01:16:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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:

~~pkg/openstack/OWNERS~~ [jichenjc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sakshi-1505 · 2023-10-22T17:45:35Z

@jichenjc @pierreprinetti can you please re-add the lgtm labels & merge this?

jichenjc · 2023-10-24T07:39:42Z

/lgtm
/hold cancel

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 8, 2023

k8s-ci-robot requested review from anguslees and FengyunPan2 October 8, 2023 07:20

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 8, 2023

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

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

dulek requested changes Oct 9, 2023

View reviewed changes

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

dulek requested changes Oct 12, 2023

View reviewed changes

sakshi-1505 force-pushed the checkListerner branch from 6b64ce3 to 122cd66 Compare October 16, 2023 14:01

sakshi-1505 requested a review from dulek October 16, 2023 14:04

sakshi-1505 force-pushed the checkListerner branch from f7a9798 to d02b997 Compare October 17, 2023 14:34

k8s-ci-robot assigned jichenjc and pierreprinetti Oct 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023

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 18, 2023

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 18, 2023

enh: add tests for checkListenerPorts

b2b02c1

sakshi-1505 force-pushed the checkListerner branch from 9cb8468 to b2b02c1 Compare October 22, 2023 17:45

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 24, 2023

k8s-ci-robot merged commit 8da2313 into kubernetes:master Oct 24, 2023

mandre pushed a commit to shiftstack/cloud-provider-openstack that referenced this pull request Feb 6, 2024

enh: add tests for checkListenerPorts (kubernetes#2405)

08cbdd9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enh: add tests for checkListenerPorts #2405

enh: add tests for checkListenerPorts #2405

sakshi-1505 commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

jichenjc commented Oct 9, 2023

sakshi-1505 commented Oct 9, 2023

dulek Oct 9, 2023

sakshi-1505 Oct 16, 2023

dulek Oct 9, 2023

sakshi-1505 Oct 10, 2023

dulek Oct 12, 2023

sakshi-1505 Oct 15, 2023

sakshi-1505 Oct 16, 2023

dulek Oct 9, 2023

sakshi-1505 Oct 16, 2023

dulek Oct 9, 2023

sakshi-1505 Oct 16, 2023

dulek Oct 12, 2023

sakshi-1505 commented Oct 16, 2023

pierreprinetti commented Oct 16, 2023

sakshi-1505 commented Oct 16, 2023

pierreprinetti commented Oct 16, 2023 •

edited

Loading

sakshi-1505 commented Oct 16, 2023 via email

pierreprinetti commented Oct 17, 2023

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

jichenjc commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

sakshi-1505 commented Oct 22, 2023

jichenjc commented Oct 24, 2023

enh: add tests for checkListenerPorts #2405

enh: add tests for checkListenerPorts #2405

Conversation

sakshi-1505 commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

jichenjc commented Oct 9, 2023

sakshi-1505 commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshi-1505 commented Oct 16, 2023

pierreprinetti commented Oct 16, 2023

sakshi-1505 commented Oct 16, 2023

pierreprinetti commented Oct 16, 2023 • edited Loading

sakshi-1505 commented Oct 16, 2023 via email

pierreprinetti commented Oct 17, 2023

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

jichenjc commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

sakshi-1505 commented Oct 22, 2023

jichenjc commented Oct 24, 2023

pierreprinetti commented Oct 16, 2023 •

edited

Loading