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