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 2 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
16 changes: 14 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ 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
MISSPELL=misspell
MISSPELL_REPO=github.com/client9/misspell
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 @@ -71,15 +78,20 @@ check: python-env
# Corrects spelling errors
.PHONY: misspell
misspell:
go get github.com/client9/misspell
${FIND} -name '*' -exec misspell -w {} \;
go get $(MISSPELL_REPO)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason you introduce a variable here?

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 tend to prefer doing this so the actual logic is more readable and easier to maintain (ideally also have $(GO)). If you don't like it I can remove it though.

Copy link
Member

Choose a reason for hiding this comment

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

I kind of worry that # of variables in the Makefile is going to explode. For me the two reasons for variables are:

  • Needs to be overwritable by beat x
  • Multiple use of the same

Perhaps we can figure out a better way to "store" the variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A different way to store would be nice, but I don't know any. Do you? Either way I'll remove this change.

Copy link
Member

Choose a reason for hiding this comment

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

The mostly constant variables like names and urls could be moved to a separate makefile and then be included by libbeat/scripts/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.

I like this idea, libbeat/scripts/Makefile-env?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer something like xyz.mk.

Copy link
Member

Choose a reason for hiding this comment

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

The nicest thing would be if we could somehow load a yaml file. So we would have things like beat_name or beat_version also available in other tools. But I don't know if there is a way in Makefiles to load env variables from a file.

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 reverted the misspell change, should we pick up this discussion in a different issue?

$(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)
$(GOLINT) ./... | $(REVIEWDOG) $(REVIEWDOG_OPTIONS)
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to give lint warnings when vendored files are updated? Do we need to filter vendor files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we do 👎


# 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 jenkins"
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
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 ./...
Copy link
Member

Choose a reason for hiding this comment

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

Does this command also need to filter out vendor? I'm not really sure what this command is used for TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I think we can just plug the whole golint command here and just run reviewdog directly. I'm not sure why its doc shows the usage as golint | reviewdog, since if you don't specify golint somewhere in the reviewdog options it complains.