Add Build/Test/Lint/go mod/vendor consistency workflow #328

martinkennelly · 2021-03-05T13:12:13Z

Signed-off-by: Kennelly, Martin [email protected]

adrianchiris · 2021-03-31T20:05:11Z

With golangci, i think its better to maintain a .golangci.yaml file in the project, then golang-ci uses it when invoked.
(that way you can also invoke it on your machine before submitting PR)
example for .golangci.yaml we use in one of our projects: https://github.com/Mellanox/network-operator/blob/master/.golangci.yml

Also i think it might be better to have this as another job within the test workflow that way we consolidate tests to a single file WDYT?

It would be great if there was also a makefile target for it so its easier for developers to test it before submitting a PR
maybe by replacing the existing make lint target as it covers golint as well.

example of Makefile target:
install golangci: https://github.com/Mellanox/network-operator/blob/eb50b92f07d54f8fd2bc878ea8b1662cbce198a4/Makefile#L114
run lint: https://github.com/Mellanox/network-operator/blob/eb50b92f07d54f8fd2bc878ea8b1662cbce198a4/Makefile#L135

Invoking it locally via Makefile can be done at a later stage though.

martinkennelly · 2021-04-27T09:25:04Z

@adrianchiris Good point. Ill rework it to what you suggested.

martinkennelly · 2021-05-01T08:59:12Z

Will add the makefile updates in a following PR. Thanks for the great input @adrianchiris. Combining build, test, lint it into one workflow is more ergonomic and gives you an overview on a single 'webpage'.

adrianchiris · 2021-06-30T12:55:57Z

@martinkennelly will you be able to add another commit to fix the issues discovered in the tests you added ?

Workflow added to build binary using multiple golang versions. Also, executes unit tests and various static analysis tools. Golanglint-ci conf file added to repository to enable local testing. Signed-off-by: Kennelly, Martin <[email protected]>

Signed-off-by: Martin Kennelly <[email protected]>

Disabled golint due to currently in archive mode. Sorted imports correctly. Correctly grammar/format mistakes. Modified go.(mod|sum) to add context. Signed-off-by: Martin Kennelly <[email protected]>

Executed go mod vendor to ensure vendor dir is consistent with go.mod. Signed-off-by: Martin Kennelly <[email protected]>

Signed-off-by: Martin Kennelly <[email protected]>

martinkennelly · 2021-07-24T16:48:40Z

@adrianchiris Force push was re-basing against master.
I've reduced 95% of the issues in the static scan outputs. Further reduction will need separate PRs and discussion. If you notice anymore low hanging fruit, let me know.

In the commit "Partial fix for static analysis errors", I needed to add "context" to the go mod file to fix an issue. I then executed go mod tidy in that same commit.

In the next commit, I then added the changes from go mod vendor to a separate commit which may aid your review of all the static scan fixes. Enjoy the review :)...

adrianchiris · 2021-07-25T15:02:59Z

pkg/netdevice/netDeviceProvider.go

@@ -104,16 +106,16 @@ func hasDefaultRoute(pciAddr string) (bool, error) {
 	}

 	if len(ifNames) > 0 { // there's at least one interface name found
-		for _, ifName := range ifNames {
-			link, err := netlink.LinkByName(ifName)
+		for iIfName := range ifNames {


not really a fan of this, do you recall which CI check complains for the string copy here ?
i dont think these memory optimizations should be a strict requirement.

Linter message:
hugeParam: X is heavy (C bytes); consider passing it by pointer (gocritic)

yeah - I did it just to silence the CI check output. Definitely agree it shouldn't be a strict requirement since this software is just a control plane component and it's dealing with objects in memory that are relatively small mem footprint (~300bytes) and are few in number.

Do you not like it because it reduces readability? I think it's fine but happy to revert if you want.

Do you not like it because it reduces readability?

yes exactly. so essentially i prefer to ignore this check or make it less restrictive so it would work in most cases.
perhaps just set a bigger threshold for this.

from gocritic docs:

@hugeParam.sizeThreshold size in bytes that makes the warning trigger (default 80)

so maybe set this to a higher number ? (512?)

adrianchiris · 2021-07-25T15:12:43Z

pkg/types/types.go

-	DeviceType     DeviceType       `json:"deviceType,omitempty"`
-	Selectors      *json.RawMessage `json:"selectors,omitempty"`
-	SelectorObj    interface{}
+	ResourcePrefix string `json:"resourcePrefix,omitempty"` // optional resource prefix that will overwrite


nit: you can put long comments above the variable, I think its clearer that way, especially for multiline comments

adrianchiris · 2021-07-25T15:20:20Z

.github/workflows/build-test-lint.yml

+    name: Hadolint
+    steps:
+    - uses: actions/checkout@v2
+    - uses: brpaz/[email protected]


should we just ignore DL3018 for now ?
alternative is to just fix the packages we install to whatever is being installed today.
(this has its downside though...)

I don't think we should ignore it because it will reduce the need for a discussion and outcome. I will create an issue following this PR to get consensus for this issue. We may just want to ignore it..

Pros for setting it to a version:
We get a version we know that works - more deterministic.
Cons for settings it to a version:
We need to update the version before its no longer available in the package managers repo or it will break our builds.

yes +1 on opening an issue and meanwhile ignore so check passes with current state of things.

Done. Ignored following community response on the issue.

adrianchiris · 2021-07-25T15:34:31Z

looking at ShellCheck issues, they seem relatively straight forward.

most remaining golang-ci issues dont seem to be too complex, but are not instant fixes.
(some may need a localy ignore in code IMO). Im fine with following up on remaining issue.

Signed-off-by: Martin Kennelly <[email protected]>

Disabled because we are not using any complicated flags in entrypoint.sh. Did not want to replace with printf. We are set to alpines almquist shell by default in our images and ash supports "-e" flag. Signed-off-by: Martin Kennelly <[email protected]>

Signed-off-by: Martin Kennelly <[email protected]>

This partially reverts commit aa3d514 pointless memory optimisation. Signed-off-by: Martin Kennelly <[email protected]>

This is a control plane component. As such, we are moving relatively little data in memory and there is no "hot path". Strictly following the gocritic advice reduces readability when pure perf isn't that important. Signed-off-by: Martin Kennelly <[email protected]>

As a community, we decided not to pin pkg versions. If we do not pin, we consume pkg fixes. If we do not pin, we test against latest pkgs. We wish to let downstream pin if they wish. Signed-off-by: Martin Kennelly <[email protected]>

Signed-off-by: Martin Kennelly <[email protected]>

Use COPY instead of ADD per DL3020. Ignore pinning version per GH issue 368. Signed-off-by: Martin Kennelly <[email protected]>

martinkennelly · 2021-07-30T11:35:49Z

@adrianchiris I needed to bump hadolint version to get ignore option and then it showed up issues in rhel7 dockerfile. Corrected them minor issue and needed to add an ignore tag for yum to hadolint. I think we are good for merge. Thanks for all your support here.

Edit: There's a lot of commits. Don't mind squashing but there is some value in them as well.. Leave it up to you.

adrianchiris · 2021-08-01T08:06:19Z

Thanks for working on this Martin !
I will sort out remaining lint issues once we merge this.

I see Mellanox CI is failing on this PR with error deploying device plugin.
@martinkennelly did you encounter any issue running device plugin from your PR ?

flag provided but not defined: -v 10 --log_dir /var/log/sriovdp --alsologtostderr
Usage of /usr/bin//sriovdp:
  -alsologtostderr
    	log to standard error as well as files
  -config-file string
    	JSON device pool config file location (default "/etc/pcidp/config.json")
  -log_backtrace_at value
    	when logging hits line file:N, emit a stack trace
  -log_dir string
    	If non-empty, write log files in this directory
  -logtostderr
    	log to standard error instead of files
  -resource-prefix string
    	resource name prefix used for K8s extended resource (default "intel.com")
  -stderrthreshold value
    	logs at or above this threshold go to stderr
  -v value
    	log level for V logs
  -vmodule value
    	comma-separated list of pattern=N settings for file-filtered logging

Incorrectly passing args to SRIOV DP binary using exec. Ignored shellcheck error as word splitting is desirable. Globbing is disabled. Fix could be converting to an array but it is too much work for no gain. Signed-off-by: Martin Kennelly <[email protected]>

adrianchiris · 2021-08-03T11:26:55Z

Thanks Martin !

LGTM, I will followup with a fix for remaining golangci-lint issues.

martinkennelly · 2021-08-03T11:36:11Z

@martinkennelly did you encounter any issue running device plugin from your PR ?

@adrianchiris Embarrassingly, I did not. Lesson learned.

adrianchiris mentioned this pull request Mar 31, 2021

Add github actions #334

Merged

martinkennelly changed the title ~~Add static analysis GA~~ WIP: Add static analysis GA Apr 29, 2021

martinkennelly force-pushed the static_ga branch 2 times, most recently from ddd2e06 to 90a1f96 Compare May 1, 2021 08:56

martinkennelly changed the title ~~WIP: Add static analysis GA~~ Add Build/Test/Lint workflow May 1, 2021

This was referenced May 1, 2021

Add build GA #329

Closed

Add test GitHub action #332

Closed

martinkennelly changed the title ~~Add Build/Test/Lint workflow~~ Add Build/Test/Lint/go mod/vendor consistency workflow Jun 17, 2021

martinkennelly mentioned this pull request Jun 17, 2021

Update vendor directory #356

Merged

martinkennelly requested a review from adrianchiris June 30, 2021 09:38

martinkennelly and others added 4 commits July 24, 2021 17:30

Add build/test/lint workflow

7631f66

Workflow added to build binary using multiple golang versions. Also, executes unit tests and various static analysis tools. Golanglint-ci conf file added to repository to enable local testing. Signed-off-by: Kennelly, Martin <[email protected]>

Add workflow to check go mod and vendor are consistent

b1ed30c

Signed-off-by: Martin Kennelly <[email protected]>

Partial fix for static analysis errors

aa3d514

Disabled golint due to currently in archive mode. Sorted imports correctly. Correctly grammar/format mistakes. Modified go.(mod|sum) to add context. Signed-off-by: Martin Kennelly <[email protected]>

Update vendor directory

37d9a4a

Executed go mod vendor to ensure vendor dir is consistent with go.mod. Signed-off-by: Martin Kennelly <[email protected]>

martinkennelly force-pushed the static_ga branch from 71c8b6b to 37d9a4a Compare July 24, 2021 16:34

Shellcheck will ignore vendor directory

2deb0c0

Signed-off-by: Martin Kennelly <[email protected]>

adrianchiris reviewed Jul 25, 2021

View reviewed changes

martinkennelly added 5 commits July 26, 2021 18:07

Fix readability of multi-line comment

bda5e69

Signed-off-by: Martin Kennelly <[email protected]>

Remove legacy backticks & prevent globbing/splitting

af73445

Signed-off-by: Martin Kennelly <[email protected]>

Prevent globbing/splitting in entrypoint

581d77c

Signed-off-by: Martin Kennelly <[email protected]>

Correct shellcheck ignore env option

1099fa1

Signed-off-by: Martin Kennelly <[email protected]>

martinkennelly mentioned this pull request Jul 26, 2021

Do we want to set apk deps to specific versions? #368

Closed

martinkennelly requested a review from adrianchiris July 26, 2021 18:26

martinkennelly added 3 commits July 29, 2021 12:37

Revert needless memory optimization to aid readability

997d565

This partially reverts commit aa3d514 pointless memory optimisation. Signed-off-by: Martin Kennelly <[email protected]>

Ignore hadolint warning for pinning apk pkg versions

0690c55

As a community, we decided not to pin pkg versions. If we do not pin, we consume pkg fixes. If we do not pin, we test against latest pkgs. We wish to let downstream pin if they wish. Signed-off-by: Martin Kennelly <[email protected]>

martinkennelly force-pushed the static_ga branch from 528f575 to 0690c55 Compare July 30, 2021 11:08

martinkennelly added 2 commits July 30, 2021 12:13

Bump hadolint action to enable ignore option

cff7a34

Signed-off-by: Martin Kennelly <[email protected]>

Fix hadolint issues in RHEL7 dockerfile

b0447c3

Use COPY instead of ADD per DL3020. Ignore pinning version per GH issue 368. Signed-off-by: Martin Kennelly <[email protected]>

adrianchiris merged commit d0b252a into k8snetworkplumbingwg:master Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Build/Test/Lint/go mod/vendor consistency workflow #328

Add Build/Test/Lint/go mod/vendor consistency workflow #328

martinkennelly commented Mar 5, 2021

adrianchiris commented Mar 31, 2021 •

edited

Loading

martinkennelly commented Apr 27, 2021

martinkennelly commented May 1, 2021

adrianchiris commented Jun 30, 2021

martinkennelly commented Jul 24, 2021

adrianchiris Jul 25, 2021

martinkennelly Jul 26, 2021

adrianchiris Jul 27, 2021 •

edited

Loading

martinkennelly Jul 30, 2021

adrianchiris Jul 25, 2021

martinkennelly Jul 30, 2021

adrianchiris Jul 25, 2021 •

edited

Loading

martinkennelly Jul 26, 2021 •

edited

Loading

adrianchiris Jul 27, 2021

martinkennelly Jul 30, 2021

adrianchiris commented Jul 25, 2021

martinkennelly commented Jul 30, 2021 •

edited

Loading

adrianchiris commented Aug 1, 2021

adrianchiris commented Aug 3, 2021

martinkennelly commented Aug 3, 2021 •

edited

Loading

Add Build/Test/Lint/go mod/vendor consistency workflow #328

Add Build/Test/Lint/go mod/vendor consistency workflow #328

Conversation

martinkennelly commented Mar 5, 2021

adrianchiris commented Mar 31, 2021 • edited Loading

martinkennelly commented Apr 27, 2021

martinkennelly commented May 1, 2021

adrianchiris commented Jun 30, 2021

martinkennelly commented Jul 24, 2021

adrianchiris Jul 25, 2021

Choose a reason for hiding this comment

martinkennelly Jul 26, 2021

Choose a reason for hiding this comment

adrianchiris Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

martinkennelly Jul 30, 2021

Choose a reason for hiding this comment

adrianchiris Jul 25, 2021

Choose a reason for hiding this comment

martinkennelly Jul 30, 2021

Choose a reason for hiding this comment

adrianchiris Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

martinkennelly Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

adrianchiris Jul 27, 2021

Choose a reason for hiding this comment

martinkennelly Jul 30, 2021

Choose a reason for hiding this comment

adrianchiris commented Jul 25, 2021

martinkennelly commented Jul 30, 2021 • edited Loading

adrianchiris commented Aug 1, 2021

adrianchiris commented Aug 3, 2021

martinkennelly commented Aug 3, 2021 • edited Loading

adrianchiris commented Mar 31, 2021 •

edited

Loading

adrianchiris Jul 27, 2021 •

edited

Loading

adrianchiris Jul 25, 2021 •

edited

Loading

martinkennelly Jul 26, 2021 •

edited

Loading

martinkennelly commented Jul 30, 2021 •

edited

Loading

martinkennelly commented Aug 3, 2021 •

edited

Loading