-
Notifications
You must be signed in to change notification settings - Fork 42
fixes: enable golanci-lint in CI, fix complaints. #232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
And hook it into [pre-]pr-checks. Signed-off-by: Krisztian Litkey <[email protected]>
2a8ff38
to
776e36e
Compare
776e36e
to
6b920fa
Compare
- name: golangci-lint | ||
uses: golangci/golangci-lint-action@v6 | ||
with: | ||
version: v1.61.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to specify the version, or should we just use the "default"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what would make the most sense here. I simply took the provided sample workflow as a basis for this, then bumped the version to the latest release, which I then also used locally for linting.
Looking at the action implementation, it is stated that we could use an explicit 'latest' here. But TBH, I'm not too keen on a setup where we are not in control so without us changing anything, things already merged might start getting flagged in new PRs as linting errors.
Also looking at other projects, most of them seem to lock the version explicitly to one of their choice, which they then keep gradually updating. If we really wanted to go with an uncontrolled rolling version, then we'd need to add a nightly run, to catch extra-repo (IOW golangci-lint) changes breaking things for us...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @klihub here. To have a reproducible builds (tests in this case) we need to lock lint version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good @klihub.
One thought is that for cases where we do record but ignore the errors, we should update the public API documentation to indiate that GetErrors()
can be queried to return errors that were raised when refreshing the cache.
Out of scope: we could also look at further segmenting the API to functions that are used by our container runtime consumers (basically Cache instantiation and InjectDevices) so that we have more flexibility with other utility functions such as getting lists of devices.
@elezar I updated the doc strings of the relevant Cache public API functions, with a brief mention a possible implicit cache refresh and how to obtain any errors encountered it. PTAL. |
Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
Update public API documentation to point out a possible implicit cache refresh and how to query any errors encountered during it. Signed-off-by: Krisztian Litkey <[email protected]>
ef77ab5
to
84ea77f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
This patch set