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.

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

enable lint golint and fix notifications #1291

Merged
merged 1 commit into from
Jan 17, 2020
Merged

enable lint golint and fix notifications #1291

merged 1 commit into from
Jan 17, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Jan 9, 2020 •
edited

Description

  • Enable golint check
  • Fix issues /(Clean code)

Closes: #1181

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2020
@camilamacedo86
Copy link
Member Author

/assign @droot

@camilamacedo86
Copy link
Member Author

/assign @mengqiy

@camilamacedo86 camilamacedo86 changed the title enable lint golint and fix notifications WIP: enable lint golint and fix notifications Jan 9, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2020
@camilamacedo86 camilamacedo86 changed the title WIP: enable lint golint and fix notifications enable lint golint and fix notifications Jan 9, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020
Adirio
Adirio reviewed Jan 10, 2020
View reviewed changes
Copy link
Contributor

@Adirio Adirio left a comment •
edited

Choose a reason for hiding this comment

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

This PR is doing several changes that were being tracked under #1176:

1- Remove Prometheus prefix inside the prometheus package: tracking issue: #1181
2- Error string formatting: tracking issue #1177, implemented in #1184 but missed that .
3- Unification of receiver names: tracking issue #1182, pending PR #1186.
4- else blocks after if blocks that end with a return.

Additionally you are introducing some golint:nolint markers which I really would try to avoid.

pkg/scaffold/scaffold.go Show resolved Hide resolved
pkg/scaffold/v1/crd/crd_sample.go Show resolved Hide resolved
test/e2e/utils/kubectl.go Show resolved Hide resolved
@Adirio

This comment has been minimized.

Sign in to view
mengqiy
mengqiy reviewed Jan 10, 2020
View reviewed changes
pkg/scaffold/v2/prometheus/monitor.go Outdated Show resolved Hide resolved
test/e2e/utils/test_context.go Show resolved Hide resolved
@mengqiy
Copy link
Member

mengqiy commented Jan 10, 2020

Please rebase.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2020
mengqiy
mengqiy reviewed Jan 10, 2020
View reviewed changes
cmd/edit.go Outdated Show resolved Hide resolved
mengqiy
mengqiy reviewed Jan 10, 2020
View reviewed changes
pkg/scaffold/api.go Outdated Show resolved Hide resolved
Adirio
Adirio suggested changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

$ ./scripts/verify.sh
running go vet
running go fmt
running golangci-lint
pkg/scaffold/scaffold.go:103:1: receiver name should not be an underscore, omit the name if it is unused (golint)
func (_ *Scaffold) validate(file input.File) error {
^
The command "./scripts/verify.sh" exited with 1.

I guess you need to change line 103 to:

func (*Scaffold) validate(file input.File) error {

Otherwise LGTM.

@Adirio
Copy link
Contributor

Adirio commented Jan 14, 2020

LGTM

Please add "Closes #1181" to the top so that the issue gets closed when this PR gets merged.

@Adirio Adirio mentioned this pull request Jan 14, 2020
Closed
41 tasks
@mengqiy
Copy link
Member

mengqiy commented Jan 17, 2020

Please squash your commits please

mengqiy
mengqiy approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@mengqiy mengqiy 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 Jan 17, 2020
@mengqiy
Copy link
Member

mengqiy commented Jan 17, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mengqiy

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 17, 2020
@mengqiy
Copy link
Member

mengqiy commented Jan 17, 2020

This one needs rebase

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2020
@Adirio Adirio mentioned this pull request Jan 17, 2020
Merged
@k8s-ci-robot k8s-ci-robot 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 Jan 17, 2020
@camilamacedo86
Copy link
Member Author

@mengqiy done the rebase.
Needs /lgtm again :-)

mengqiy
mengqiy approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@mengqiy mengqiy 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 Jan 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1bbfd06 into kubernetes-sigs:master Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adirio Adirio Adirio requested changes

@mengqiy mengqiy mengqiy approved these changes

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@droot droot

@mengqiy mengqiy

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Package name as exported variable/type/function prefix removal
5 participants
@camilamacedo86 @Adirio @mengqiy @k8s-ci-robot @droot