And to be sure, Michael's roll out timeline is exactly right. Add a container that runs all the applicable linters, outputs the test results, and then exits 0, regardless of the result. That makes the build succeed, but produces the error list so we can work on it. Once we get it all cleared out, we change the exit code to match that of the linters.
The gofmt check should just be a run of `gofmt -d` and verify that it's empty. If it is non-empty, then something needs to be formatted. (And the diff will list the files that need it.) On Tue, Oct 1, 2019 at 4:32 PM ocket 8888 <[email protected]> wrote: > > I don't think we should get bogged down with the configuration right now, > that should appear in a PR at some point which will be much easier to use > as a platform for the debate. > > I don't mean to suggest that `pkg` wouldn't run the linter, just that its > doing so would be as a consequence of building something. IMO, if we're > going to enforce a linter, it ought to be a part of the build, because code > that doesn't pass a mandatory linter is therefore wrong. > > On Tue, Oct 1, 2019, 16:19 Robert Butts <[email protected]> wrote: > > > I think `pkg` is the right place to put this. > > > > `pkg` isn't just for building. I think it has that name because it's short, > > not because it's limited to "packaging." It also does things like the > > license checking via Weasel. It's more like `make`, which also isn't > > limited to building. It's really for anything that's a series of steps. It > > gives us a unified place to tell people to do things against the project > > code. Like "To run unit tests, in the root of the project run ./pkg > > traffic_ops_unit_tests." > > > > Also, +1 on linting. > > > > Though I'd rather see it not strictly enforced, but rather used as a > > guideline. IMO it's already hard enough for new people to start > > contributing. But I've made my opinion clear and been outvoted in the past, > > so I'll remain a -0 on strict enforcement and not rock the boat. > > > > -1 on ineffassign. It complains about initializing variables. Explicit is > > better than Implicit. Even if a variable is immediately re-assigned, it's > > safer to initialize explicitly. I'd rather see us standardize around `:=`, > > and never use `var`, as it's both implicit and obscures the value. There > > should also be One Right Way, where `var` unnecessarily creates two ways of > > declaring variables. I see ineffassign as removing a safety, and making our > > code more dangerous, and more inconsistent, not less. > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
