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

dev-tools: run golint/reviewdog in Jenkins #3832

Merged
merged 7 commits into from
Apr 4, 2017
Merged

dev-tools: run golint/reviewdog in Jenkins #3832

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
40dec92
dev-tools: run golint/reviewdog in Jenkins
Mar 28, 2017
c8f6375
jenkins_ci: map github plugin vars to reviewdog
Mar 28, 2017
fd5ebef
Make lint target available in other beats
Mar 29, 2017
f27044c
Run reviewdog directly
Mar 30, 2017
21e15af
Introduce dummy warning to test reviewdog
Mar 31, 2017
0ffc2d4
Rename -ci option to "common" (that is apparently what jenkins falls …
Mar 31, 2017
ae01b90
Remove dummy warning
Mar 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ SNAPSHOT?=yes
PYTHON_ENV?=${BUILD_DIR}/python-env
VIRTUALENV_PARAMS?=
FIND=find . -type f -not -path "*/vendor/*" -not -path "*/build/*" -not -path "*/.git/*"
GOLINT=golint
Copy link
Member

Choose a reason for hiding this comment

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

I think these variables are not needed anymore here because they are in the libbeat Makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still have a lint target in this Makefile and I don't see it including the one from libbeat/scripts, did I miss that?

Copy link
Member

Choose a reason for hiding this comment

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

my fault, I assume this lint target calls all the other lint targets like we do for update and other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The reason is I didn't want to risk running it on overlapping code, nor keep track of what's a beat and what's not. So if you run it at the top level it runs on everything, if you run it inside a beat it runs only on the beat.

Copy link
Member

Choose a reason for hiding this comment

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

Ok for me. My main worry is that we have quite a few duplicated lines and will change them in the future in one place but not the other.

GOLINT_REPO=github.com/golang/lint/golint
REVIEWDOG=reviewdog
REVIEWDOG_OPTIONS?=-diff "git diff master"
REVIEWDOG_REPO=github.com/haya14busa/reviewdog/cmd/reviewdog

# Runs complete testsuites (unit, system, integration) for all beats with coverage and race detection.
# Also it builds the docs and the generators
Expand Down Expand Up @@ -72,14 +77,19 @@ check: python-env
.PHONY: misspell
misspell:
go get github.com/client9/misspell
${FIND} -name '*' -exec misspell -w {} \;
$(FIND) -name '*' -exec misspell -w {} \;

.PHONY: fmt
fmt: python-env
$(foreach var,$(PROJECTS),$(MAKE) -C $(var) fmt || exit 1;)
# Cleans also python files which are not part of the beats
find . -type f -name *.py -not -path "*/vendor/*" -not -path "*/build/*" -not -path "*/.git/*" -exec autopep8 --in-place --max-line-length 120 {} \;

.PHONY: lint
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to be able to run make lint for any individual Beat and for community Beats. Would it be possible to have a lint target in libbeat/scripts/Makefile that's is common to all Beats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be nice to mimic what Jenkins does, i.e., run it on the diff. Are you suggesting running it on everything by default or having the ability to restrict it to the diff of a single beat?

Copy link
Member

Choose a reason for hiding this comment

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

I'm suggesting moving the implementation of the lint target to libbeat/scripts/Makefile. This provides two things:

  • Developers of community beats can run make lint to see new linter warnings that they introduced while making changes.
  • Developers of elastic/beats can call make lint inside of a beat sub-dir and the operation is limited to changes to that specific beat.

I would still want to have the ability to run make lint at the top level in elastic/beats to check the whole repo. This is consistent with other commands like make fmt or make check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool that makes sense, thanks!

lint:
@go get $(GOLINT_REPO) $(REVIEWDOG_REPO)
$(REVIEWDOG) $(REVIEWDOG_OPTIONS)

# Collects all dashboards and generates dashboard folder for https://github.com/elastic/beats-dashboards/tree/master/dashboards
.PHONY: beats-dashboards
beats-dashboards:
Expand Down
10 changes: 10 additions & 0 deletions dev-tools/jenkins_ci
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,21 @@ parse_args() {
fi
}

lint() {
if [ -z $ghprbPullId ]; then
return
fi
export REVIEWDOG_OPTIONS="-ci common"
IFS='/' read -a repo <<< "$ghprbGhRepository"
CI_PULL_REQUEST=$ghprbPullId CI_COMMIT=$ghprbActualCommit CI_REPO_OWNER=${repo[0]} CI_REPO_NAME=${repo[1]} make lint
}

build() {
make check
make testsuite
make coverage-report
make docs
lint
}

cleanup() {
Expand Down
2 changes: 1 addition & 1 deletion filebeat/prospector/prospector_log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type Log struct {
config prospectorConfig
}

// NewLog instantiates a new Log
// NewProspectorLog instantiates a new Log
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported function NewLog should be of the form "NewLog ..."

func NewLog(p *Prospector) (*Log, error) {

prospectorer := &Log{
Expand Down
10 changes: 10 additions & 0 deletions libbeat/scripts/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ COVERAGE_DIR=${BUILD_DIR}/coverage
COVERAGE_TOOL=${BEAT_GOPATH}/bin/gotestcover
COVERAGE_TOOL_REPO=github.com/elastic/beats/vendor/github.com/pierrre/gotestcover
TESTIFY_TOOL_REPO=github.com/elastic/beats/vendor/github.com/stretchr/testify
GOLINT=golint
GOLINT_REPO=github.com/golang/lint/golint
REVIEWDOG=reviewdog -conf ${ES_BEATS}/reviewdog.yml
REVIEWDOG_OPTIONS?=-diff "git diff master"
REVIEWDOG_REPO=github.com/haya14busa/reviewdog/cmd/reviewdog
PROCESSES?= 4
TIMEOUT?= 90
NOSETESTS_OPTIONS?=--process-timeout=$(TIMEOUT) --with-timer ## @testing the options to pass when calling nosetests
Expand Down Expand Up @@ -96,6 +101,11 @@ fmt: python-env ## @build Runs `gofmt -s -l -w` and `autopep8`on the project's s
gofmt -s -l -w ${GOFILES_NOVENDOR}
${FIND} -name *.py -exec autopep8 --in-place --max-line-length 120 {} \;

.PHONY: lint
lint:
@go get $(GOLINT_REPO) $(REVIEWDOG_REPO)
$(REVIEWDOG) $(REVIEWDOG_OPTIONS)

.PHONY: clean
clean:: ## @build Cleans up all files generated by the build steps
rm -rf build
Expand Down
3 changes: 3 additions & 0 deletions reviewdog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
runner:
golint:
cmd: golint $(go list ./... | grep -v /vendor/)