> but we shouldn't forget the java code when we're talking about linting. I've used pmd on java in the distant past, to reasonable effect, but I don't have a super-strong opinion on the matter.
FWIW we do use PMD for Traffic Router, but I don't know off the top of my head if it's checked on every maven target or just the test/verify targets. I mostly just run `mvn clean verify` and `mvn clean test`, and both of those will spit out any PMD violations and return a nonzero exit code. In general I think we need to get our automated testing more in shape to where we're running all the tests for every PR opened or updated -- not just the `pkg` build. Reviewers shouldn't have to run all the unit tests, integration tests, linters, etc. themselves before deciding to merge a PR. If jenkins can just give us a thumbs up or a thumbs down, that would save a lot of manual labor. And, as we see all the time, _some people_ (me) forget to always run unit tests manually and things slip by. Robots don't forget those things. - Rawlin On Tue, Oct 1, 2019 at 3:59 PM Chris Lemmons <[email protected]> wrote: > > Yeah, that's the name, but all it does is start the jobs in > infrastructure/docker/build/docker-compose.yml . Adding it to the > build infrastructure is the right way to make it easy for everyone to > run it in the exact same way, including the CI. > > The pkg script is just a helper that reduces the requirements for > building so that you don't have to keep docker-compose on hand, > handles the return code of the processes, and organizes the log output > into something you can read. > > golangci-lint is the gold standard metalinter for the community. In > general, if it's complaining about something, it's probably doing so > with good reason. It is the blessed replacement for gometalinter, > which was the previous gold standard. I'm +1 on golangci-lint. > > We will want to have additional discussions about which specific > linters we turn on or off. Some of our existing frequent practices are > at odds with a few of the linters and we'll want to decide whether our > practices or the linter's suggestions are better. (Shadowed variables, > I'm looking at you. :) ) > > I like having it output the errors as junit-xml for the CI, because > then we can list that as a unit test file and it will make a great > interface for seeing what is failing and when. ("Unit test" is a bit > of a misnomer here, but all the concepts map just fine. Call it a > "static unit test" or something.) > > Add it as an item in the build/docker-compose and people can run it > manually or just as part of a build. We don't really want to encourage > skipping the static analysis when doing a full build, anyway. (It is, > of course, still possible, the same way you can skip the weasel if you > really wanted.) > > We've also got java and perl. Perl is on its way out, so I don't know > that adding a linter and fixing issues there is a great use of energy, > but we shouldn't forget the java code when we're talking about > linting. I've used pmd on java in the distant past, to reasonable > effect, but I don't have a super-strong opinion on the matter. > > On Tue, Oct 1, 2019 at 3:17 PM ocket 8888 <[email protected]> wrote: > > > > I agree, linting shouldn't be a part of package building. > > > > On Tue, Oct 1, 2019 at 12:26 PM Hoppal, Michael <[email protected]> > > wrote: > > > > > `pkg` seems like a weird location for a linter to me. It doesn’t have > > > anything to do with packaging/building of the services. > > > > > > https://github.com/apache/trafficcontrol/tree/master/infrastructure/test > > > seems like a better place to put the linter in. > > > > > > Michael > > > > > > On 10/1/19, 12:00 PM, "Dan Kirkwood" <[email protected]> wrote: > > > > > > It really should only be an addition to > > > `infrastructure/docker/build/docker-compose.yml` as `pkg` just passes > > > its > > > arguments to `docker-compose`. > > > > > > -dan > > > > > > On Tue, Oct 1, 2019 at 10:33 AM Gray, Jonathan < > > > [email protected]> > > > wrote: > > > > > > > How do you think the linter process would integrate with our > > > existing > > > > ./pkg wrapper if at all? > > > > > > > > Jonathan G > > > > > > > > On 10/1/19, 10:24 AM, "Rawlin Peters" <[email protected]> > > > wrote: > > > > > > > > +1, I'm generally in favor of shared linters and formatters > > > where > > > > possible, and that rollout path sounds good to me. > > > > > > > > - Rawlin > > > > > > > > On Mon, Sep 30, 2019 at 8:28 AM Hoppal, Michael > > > > <[email protected]> wrote: > > > > > > > > > > Hi, > > > > > > > > > > As we grow our Golang footprint within ATC we should consider > > > the > > > > addition of a linter for our CI. > > > > > > > > > > As with any linter it provides a lot of benefits including > > > enforcing > > > > a consistent style, early detection of potential bugs and speed up > > > of PR > > > > reviews. > > > > > > > > > > That being said I propose that we add the linter > > > GoLangCI-Lint< > > > > > > > https://protect2.fireeye.com/url?k=bd4100fc-e1a50e37-bd412748-000babff3540-012a587242ed1320&u=https://github.com/golangci/golangci-lint> > > > to our CI. It wraps many > > > > widely used linters in the Golang opensource community with the > > > ability to > > > > turn on which ones are run. It also supports outputting results in > > > > checkstyle which is consumable via Jenkins for a visual report. > > > > > > > > > > To start I would recommend to stay with the default enabled > > > linters< > > > > > > > https://protect2.fireeye.com/url?k=9e1436b4-c2f0387f-9e141100-000babff3540-944aacd53629deee&u=https://github.com/golangci/golangci-lint#enabled-by-default-linters> > > > on > > > > the tool with the addition of Gofmt. > > > > > > > > > > The roll out path (up for discussion of course) would be: > > > > > > > > > > > > > > > * Makefile target within the source code to allow > > > developers to > > > > run the linting locally as they develop > > > > > * Inclusion of GolangCI-Lint within CI as a non-voting > > > component > > > > on every PR (as to not block development when turned on) > > > > > * Fixing of the current lint violations > > > > > * Make the linting a blocking voting component of CI > > > > > > > > > > What are peoples thoughts on the inclusion of linting in > > > general, > > > > choice of linter and the outlined rollout plan? > > > > > > > > > > Thanks, > > > > > > > > > > Michael Hoppal > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
