-
Notifications
You must be signed in to change notification settings - Fork 210
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
Remove unused code #755
base: master
Are you sure you want to change the base?
Remove unused code #755
Conversation
This issue is currently awaiting triage. If the repository mantainers determine this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
24f22ca
to
d40c9de
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: panslava The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
d40c9de
to
d70649e
Compare
/test cloud-provider-gcp-verify-up-to-date |
it fails to compile now, hence the jobs are failing, you may need to run |
d70649e
to
7403d16
Compare
oh I accidentally commited next work, that was not supposed to go in this commit, fixing |
was this done using static analysis? |
@bowei yes, I used golangci-lint "unused" linter, I can add it to the repo |
@@ -72,7 +72,6 @@ var ( | |||
authSyncNodeURL = pflag.String("auth-sync-node-url", "", "URL for reaching the Auth Service SyncNode API.") | |||
hmsAuthorizeSAMappingURL = pflag.String("hms-authorize-sa-mapping-url", "", "URL for reaching the Hosted Master Service AuthorizeSAMapping API.") | |||
hmsSyncNodeURL = pflag.String("hms-sync-node-url", "", "URL for reaching the Hosted Master Service SyncNode API.") | |||
autopilotEnabled = pflag.Bool("autopilot", false, "Is this a GKE Autopilot cluster.") |
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.
if you remove flags, independently they maybe a noop, the manifests using this binary need to be updated or will fail to execute
I'm not much of a fun of removing code that is unused just because of that, it is not we are charged by number of lines and there is always something that may go wrong ... do we really need to take this risk? |
7403d16
to
012f38f
Compare
yeh, tbh, initially I thought that would help me for further stuff, but not really I remade this pr and ran linter with slightly softer (or stricter lol) rules so now I removed code that should not break anything. Also added linter config but wouldn't say it's really needed, so feel free to ignore and close |
012f38f
to
f63dfe7
Compare
We should try to delete dead code (otherwise when we will do that?) but flags we need to mark as deprecated or whatnot and keep for a while. |
f63dfe7
to
c3f022a
Compare
- Add .golangci.yaml config with only unused linter enabled
c3f022a
to
b5ced24
Compare
okay then
|
linters: | ||
# Disable all linters. | ||
# Default: false | ||
disable-all: true |
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.
do we lose coverage?
we were running the linters before AFAIK as part of https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/cloud-provider-gcp/755/cloud-provider-gcp-verify-all/1828578606167625728
PR needs rebase. 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-sigs/prow repository. |
/assign @aojea
This shouldn't change any public interfaces, or any functionality, just never used code removed