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