Skip to content

remove TODO in test/e2e/framework/providers/gce/ingress.go #87132

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
Merged
merged 1 commit into from
Jan 15, 2020
Merged

remove TODO in test/e2e/framework/providers/gce/ingress.go #87132

merged 1 commit into from
Jan 15, 2020

Conversation

yuxiaobo96
Copy link
Contributor

Signed-off-by: yuxiaobo yuxiaobogo@163.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

ref:#86763

Special notes for your reviewer:

The TODO in the function GetFirewallRuleOrError is related to the function GetFirewallRule. But the function GetFirewallRule is not used, so remove this function and modify the function GetFirewallRuleOrError to GetFirewallRule.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 13, 2020
@yuxiaobo96
Copy link
Contributor Author

/test pull-kubernetes-integration

@yuxiaobo96
Copy link
Contributor Author

/retest

neolit123
neolit123 reviewed Jan 13, 2020
View reviewed changes
test/e2e/framework/providers/gce/ingress.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

according to github, this function is unused, so the change seems fine.

neolit123
neolit123 reviewed Jan 13, 2020
View reviewed changes
test/e2e/framework/providers/gce/ingress.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

this is a good change as there is no need to have OrError for a function that returns an error.

/lgtm
/approve

Copy link
Member

Choose a reason for hiding this comment

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

the function still needs an export comment on top of it?

// GetFirewallRule returns the firewall used by the IngressController.
// Returns an error if that fails.
func (cont *IngressController) GetFirewallRule() (*compute.Firewall, error) {
...

Copy link
Contributor Author

@yuxiaobo96 yuxiaobo96 Jan 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is on line 573 and is correct.

image

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2020
@neolit123
Copy link
Member

please fix this error:

Errors from golint:
test/e2e/framework/providers/gce/ingress.go:573:1: comment on exported method IngressController.GetFirewallRule should be of the form "GetFirewallRule ..."

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 13, 2020
@yuxiaobo96
Copy link
Contributor Author

@neolit123
Thanks for your review.
done

@yuxiaobo96
remove TODO in test/e2e/framework/providers/gce/ingress.go
cb07680 
Signed-off-by: yuxiaobo <yuxiaobogo@163.com>
@yuxiaobo96
Copy link
Contributor Author

/retest

@oomichi
Copy link
Member

oomichi commented Jan 14, 2020

unrelated test is failed.
The change itself is good for me.

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2020
@yuxiaobo96
Copy link
Contributor Author

/retest

@tanjunchen
Copy link
Contributor

/lgtm
i think it is good

@oomichi
Copy link
Member

oomichi commented Jan 15, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, oomichi, yuxiaobo96

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 Jan 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit 2da2c3b into kubernetes:master Jan 15, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcbw dcbw Awaiting requested review from dcbw

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

1 more reviewer

@neolit123 neolit123 neolit123 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@neolit123 neolit123

@oomichi oomichi

@tanjunchen tanjunchen

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test 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. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v1.18

Development

Successfully merging this pull request may close these issues.

5 participants

@yuxiaobo96 @neolit123 @oomichi @tanjunchen @k8s-ci-robot