Conversation
|
Can one of the admins verify this patch? |
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
This is very cool, I was not aware of gometalinter.
| visibility = ["//visibility:public"], | ||
| ) | ||
|
|
||
| filegroup( |
There was a problem hiding this comment.
Does this need to be added to every sub-package, or could it be a top level definition?
There was a problem hiding this comment.
Afaik it's needed. I can't find a way to filegroup crossing target boundaries, but my knowledge here is limited, would love to hear any ideas.
| testonly = 1, | ||
| ) | ||
|
|
||
| sh_test( |
There was a problem hiding this comment.
You could use our custom gen_test here, instead of generating a shell script via a genrule. gen_test is in no way perfect, but might be a bit less verbose.
installer/BUILD.bazel
Outdated
|
|
||
| sh_test( | ||
| name = "cli_gometalinter", | ||
| size = "large", |
There was a problem hiding this comment.
large is a bit exaggerated, resulting in: There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.. Would you mind adjusting it to a more reasonable level? (See Bazel level docs: https://docs.bazel.build/versions/master/test-encyclopedia.html#role-of-the-test-runner)
installer/BUILD.bazel
Outdated
| installer/pkg/config/vmware \ | ||
| installer/pkg/config-generator \ | ||
| installer/pkg/terraform-generator \ | ||
| installer/pkg/workflow", |
There was a problem hiding this comment.
Can we add the validate package? it is newly created to be used in the CLi for valdation: https://github.com/coreos/tectonic-installer/tree/master/installer/pkg/validate
installer/BUILD.bazel
Outdated
| ) | ||
|
|
||
| sh_test( | ||
| name = "cli_gometalinter", |
There was a problem hiding this comment.
can we remove the cli_ prefix from these tests? I think it is clear enough with bazel test installer:gometalinter or even just bazel test installer:lint
enxebre
force-pushed
the
gometalinter
branch
2 times, most recently
from
d1da3ff to
04fd8e7
Compare
|
@enxebre Is there a way to include the go smoke test code in the linting process as well? |
|
thanks for the suggestion @mxinden, added a new commit with it and dupl improvements |
|
ok to test |
enxebre
force-pushed
the
gometalinter
branch
3 times, most recently
from
ef58ae9 to
68f73bf
Compare
| --enable=gocyclo \ | ||
| --enable=misspell \ | ||
| --enable=dupl \ | ||
| installer/cmd/tectonic \ |
There was a problem hiding this comment.
I worry that this is not flexible enough. Next time somebody adds a new package that has broken linting but forgets to add it here we will never know. We should use go list to grab all the packages.
There was a problem hiding this comment.
Yeah, I don't really like it much either...
A new package would need to add the filegroup
filegroup( name = "go_files", visibility = ["//visibility:public"], srcs = glob([ "*.go", ]), )
And then it has to be referenced here.
However I couldn't find nicer way to collect all the go files crossing target boundaries with Bazel.
For using something like go lint, wouldn't the packages need to be made available for it during bazel run time? so wouldn't it still require manual static reference of the packages?
I can't see a way in bazel to "collect all the files" without adding static targets
|
needs rebase |
|
@enxebre still doing this? If not please close. |
6 participants
Bazel support for gometalinter