Add stylecheck linter to golangci-lint CI runs #1125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcantah
force-pushed
the
add-stylecheck
branch
from
faf3e78 to
35af99c
Compare
|
fyi @SeanTAllen |
|
@dcantah awesome. |
dcantah
force-pushed
the
add-stylecheck
branch
from
35af99c to
7a94382
Compare
|
Could we have a separate job that runs just the |
|
Alternatively, this example mentions that you can exclude things per linter if I'm reading it correctly. |
|
@katiewasnothere If that issues section supports regexes, we can exclude anything with s1003 under the schema dirs. That sounds grand to me |
dcantah
force-pushed
the
add-stylecheck
branch
from
5752640 to
961cf1c
Compare
|
@katiewasnothere That did the trick :). @SeanTAllen About two or three fixes for some of the security policy stuff out of ST1003 |
This change adds the stylecheck linter to our golangci-lint ci run. This catches a few nice things like double imports, checking if errors end with punctuation/aren't capitalized. It also by default checks if common initialisms (CPU, ID, HTTP, TLS) are in all caps, but this sets off a metric ton of errors because of the amount of generated schema files we have. We could exclude these directories from being linted altogether, but would like to hear what others think. I don't see a way to exclude directories for only certain checks (so if someone knows a way please do tell) Signed-off-by: Daniel Canter <[email protected]>
This change fixes the issues that the stylecheck linter for golangci-lint brings up. Most of them are errors starting with a capital letter/ending with punctuation, but there's two that are for double imports. Also adds -v to the golangci-lint args, so it's easier to tell what's going on. In internal/layers/layers.go we were importing the /internal/uvm package twice, as we used any constants from the package under a package alias of uvmpkg and then any uses of a pointer to a UtilityVM object were passed around as `uvm`. I've changed the latter to be passed around via vm, as we use this elsewhere to store a UtilityVM object, and then simply replaced umvpkg with the package name itself, uvm. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
add-stylecheck
branch
2 times, most recently
from
e655ed4 to
5670340
Compare
ST1003 checks for if any initialisms don't follow the Go idiom of all capitals. However, due to this repo having a really high number of packages that have OS bindings/constants, generated schema files that we generally try and not touch, and some calls that have been exported and used forever so changing them would introduce a breaking change, I've taken to excluding some directories from this specific check. Stylecheck should still check for double imports, and any error message shenanigans. This change additionally fixes all of the ST1003 errors in the packages that aren't excluded. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
add-stylecheck
branch
from
5670340 to
d7cdc0f
Compare
|
This is a great PR even if it probably means I'm going to have some merge conflicts. |
dcantah
commented
Aug 27, 2021
| @@ -12,7 +12,7 @@ import ( | |||
| const ( | |||
| hcsComputeSystemSaveType = "AsTemplate" | |||
| // default namespace ID used for all template and clone VMs. | |||
| DEFAULT_CLONE_NETWORK_NAMESPACE_ID = "89EB8A86-E253-41FD-9800-E6D88EB2E18A" | |||
| DefaultCloneNetworkNamespaceID = "89EB8A86-E253-41FD-9800-E6D88EB2E18A" | |||
There was a problem hiding this comment.
@ambarve Was there a reason this was screaming case? It's not an actual constant from the OS iirc right?
There was a problem hiding this comment.
There was no reason to have this in capital case. It is fine to change this.
|
lgtm. Thanks a lot for working on this Danny! |
ambarve
approved these changes
Sep 1, 2021
anmaxvl
pushed a commit
to anmaxvl/hcsshim
that referenced
this pull request
Nov 17, 2021
Related work items: microsoft#1062, microsoft#1087, microsoft#1089, microsoft#1095, microsoft#1104, microsoft#1112, microsoft#1117, microsoft#1118, microsoft#1125, microsoft#1137, microsoft#1139, microsoft#1140, microsoft#1141, microsoft#1142, microsoft#1143, microsoft#1145, microsoft#1146, microsoft#1150, microsoft#1151, microsoft#1153, microsoft#1154, microsoft#1155, microsoft#1156, microsoft#1157, microsoft#1158, microsoft#1159, microsoft#1161, microsoft#1162, microsoft#1163, microsoft#1164, microsoft#1165, microsoft#1166, microsoft#1167, microsoft#1168, microsoft#1169, microsoft#1171, microsoft#1172, microsoft#1173, microsoft#1174, microsoft#1178
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 28, 2024
Add stylecheck linter to golangci-lint CI runs
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
Add stylecheck linter to golangci-lint CI runs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds the stylecheck linter to our golangci-lint ci run.
This catches a few nice things like double imports, checking if errors
end with punctuation/aren't capitalized. It also by default checks if
common initialisms (CPU, ID, HTTP, TLS) are in all caps, but this sets off a
metric ton of errors because of the amount of generated schema files we have.
We could exclude these directories from being linted altogether, but would like
to hear what others think. I don't see a way to exclude directories for only certain
checks (so if someone knows a way please do tell)
This change also fixes the issues that the stylecheck linter for golangci-lint
brings up. Most of them are errors starting with a capital letter/ending with
punctuation, but there's two that are for double imports.
In internal/layers/layers.go we were importing the /internal/uvm package twice,
as we used any constants from the package under a package alias of
uvmpkgand thenany uses of a pointer to a
UtilityVMstructt were passed around asuvm. I've changedthe latter to be passed around via
vm, as we use this elsewhere in our codebase to store aUtilityVMobject, and then simply replaced
umvpkgwith the package name itself,uvm.Signed-off-by: Daniel Canter [email protected]