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.

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

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

Merged
merged 7 commits into from Apr 4, 2017

Conversation

Projects
None yet
5 participants
@7AC
Copy link
Contributor

commented Mar 28, 2017

reviewdog allows to restrict golint runs to the current diff. It also integrates with GitHub, so it can comment inline on a PR and leave it up to the reviewers to decide wether or not the warnings are worth fixing (the build will still pass). This would allow the kind of flexibility we need in deciding which comments to add to functions etc.

@7AC 7AC added the review label Mar 28, 2017

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

For non-PR builds we could setup Jenkins to track the changes to the linter reports. This could be done by using gometalinter's checkstyle output and have Jenkins parse that report. No action needed, just a thought for the new Jenkins setup.


.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

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2017

Member

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?

This comment has been minimized.

Copy link
@7AC

7AC Mar 29, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2017

Member

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.

This comment has been minimized.

Copy link
@7AC

7AC Mar 29, 2017

Author Contributor

Cool that makes sense, thanks!

Makefile Outdated
@@ -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)

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 29, 2017

Collaborator

Is there a specific reason you introduce a variable here?

This comment has been minimized.

Copy link
@7AC

7AC Mar 29, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 30, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@7AC

7AC Mar 30, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 30, 2017

Member

The mostly constant variables like names and urls could be moved to a separate makefile and then be included by libbeat/scripts/Makefile.

This comment has been minimized.

Copy link
@7AC

7AC Mar 30, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 31, 2017

Member

I'd prefer something like xyz.mk.

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 31, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@7AC

7AC Mar 31, 2017

Author Contributor

I reverted the misspell change, should we pick up this discussion in a different issue?

Makefile Outdated
.PHONY: lint
lint:
@go get $(GOLINT_REPO) $(REVIEWDOG_REPO)
$(GOLINT) ./... | $(REVIEWDOG) $(REVIEWDOG_OPTIONS)

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2017

Member

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

This comment has been minimized.

Copy link
@7AC

7AC Mar 29, 2017

Author Contributor

Yeah we do 👎

@ruflin

ruflin approved these changes Mar 30, 2017

@@ -0,0 +1,3 @@
runner:
golint:
cmd: golint ./...

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 30, 2017

Member

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

This comment has been minimized.

Copy link
@7AC

7AC Mar 30, 2017

Author Contributor

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.

@@ -24,7 +24,7 @@ type Log struct {
config prospectorConfig
}

// NewLog instantiates a new Log
// NewProspectorLog instantiates a new Log

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine Mar 31, 2017

Collaborator

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

@@ -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

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 3, 2017

Collaborator

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

This comment has been minimized.

Copy link
@7AC

7AC Apr 3, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 3, 2017

Collaborator

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

This comment has been minimized.

Copy link
@7AC

7AC Apr 3, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 4, 2017

Collaborator

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.

@andrewkroh andrewkroh merged commit c0c7b4a into elastic:master Apr 4, 2017

6 checks passed

CLA Commit author has signed the CLA
Details
codecov/patch Coverage not affected when comparing e41868b...ae01b90
Details
codecov/project 65.88% (+0.45%) compared to e41868b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

dev-tools: run golint/reviewdog in Jenkins (elastic#3832)
* dev-tools: run golint/reviewdog in Jenkins

* jenkins_ci: map github plugin vars to reviewdog

* Make lint target available in other beats

* Run reviewdog directly

* Introduce dummy warning to test reviewdog

* Rename -ci option to "common" (that is apparently what jenkins falls into)

* Remove dummy warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.