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