Update licensing#590
Conversation
Codecov Report
@@ Coverage Diff @@
## main #590 +/- ##
==========================================
+ Coverage 61.92% 62.20% +0.27%
==========================================
Files 47 47
Lines 5742 5742
==========================================
+ Hits 3556 3572 +16
+ Misses 1958 1942 -16
Partials 228 228
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
youngnick
force-pushed
the
update-licensing
branch
5 times, most recently
from
9145083 to
e1a5f2b
Compare
danehans
left a comment
danehans
left a comment
There was a problem hiding this comment.
@youngnick thanks for making this happen. I have a few nits throughout.
There was a problem hiding this comment.
any way to reuse rather than copy code over and maintain ?
There was a problem hiding this comment.
Not easily, no. This should need minimal maintenance though, since it's only using core python features, and already does everything we need.
There was a problem hiding this comment.
nit: if we have more of these, we should consider creating check and add licencse-check and gen-check under it
There was a problem hiding this comment.
Yes, I went back and forth on breaking this out, but decided three should be the critical number. Does that seem okay?
There was a problem hiding this comment.
sg thanks for sharing the thought process
There was a problem hiding this comment.
The GNU convention is that check runs the test suite.
There was a problem hiding this comment.
On the other side, I'd argue that this should be part of the existing make lint.
There was a problem hiding this comment.
I'm going to leave this as-is for now, so that we can have proper licensing for our first big public release. I'm happy for targets to move around at a later date though.
There was a problem hiding this comment.
Since we're at the v0.2.0 release date, we can iterate to improve in v0.3.0.
There was a problem hiding this comment.
Adding these makes these imports not get removed by gofmt, so auto-format-on-save won't break them.
There was a problem hiding this comment.
gofmt won't remove them, but goimports (which is mostly a drop-in replacement for gofmt, and is what your auto-format-on-save is likely using) will.
Good change.
There was a problem hiding this comment.
But this is also going to require a change to the regex in tools.mk
- cd $(<D) && GOOS= GOARCH= go build -o $(abspath $@) $$(sed -En 's,^import "(.*)".*,\1,p' pin.go)
+ cd $(<D) && GOOS= GOARCH= go build -o $(abspath $@) $$(sed -En 's,^import _ "(.*)".*,\1,p' pin.go)There was a problem hiding this comment.
Oh, that'll teach me to work on this late. Adding the underscore breaks the whole tooling, Go doesn't consider the import relevant any more and so can't do go build, it seems. Sigh, I'll roll it back, no editing pin.go without using a separate editor for me.
There was a problem hiding this comment.
Surely the linter is complaining about all these extra newlines? (Because gofmt would remove them)
There was a problem hiding this comment.
This was an artifact of how controller-gen was working, fixed.
There was a problem hiding this comment.
This is due to a change in gofmt from 1.18 to 1.19. This should be in a separate Go 1.19 PR.
There was a problem hiding this comment.
The existing golangci-lint can do this too, https://golangci-lint.run/usage/linters/#goheader no need to introduce a bunch more Python and Bash.
There was a problem hiding this comment.
Done, converted. There are now two places to update the boilerplate though, tools/linter/golangci-lint/.golangci.yml (golangci-lint config) and tools/boilerplate.txt (controller-gen template). The former expects no comment marks, while the latter requires them.
There was a problem hiding this comment.
After doing this and rebasing, there were two files missing headers, but the golangci-lint header check doesn't tell you the names, just that there are files missing headers, when it runs in Github Actions, anway. I spent about an hour poking at it, but I've reverted to boilerplate.py, since that actually has useful error messages.
If we want to change it later, that's fine, but someone else will need to have a crack.
youngnick
force-pushed
the
update-licensing
branch
4 times, most recently
from
efbd344 to
df00765
Compare
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
youngnick
force-pushed
the
update-licensing
branch
from
df00765 to
fd6ba26
Compare
Signed-off-by: Nick Young <nick@isovalent.com>
5 participants
This PR adds license headers and license checks as discussed in #92.
It also adds a CI check for license headers.
It's a pretty big PR, sorry, but I had to touch every single Go file in the repo.
I've left it to only Go files for now, the tool can do YAML files as well, but I figured this was big enough already.
Closes #92.