Skip to content
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

Introduce static code analysis #50

Merged
merged 3 commits into from
Jan 15, 2024
Merged

Introduce static code analysis #50

merged 3 commits into from
Jan 15, 2024

Conversation

mhmxs
Copy link
Contributor

@mhmxs mhmxs commented Jan 2, 2024

Golangci-lint, Gosec, Govulncheck and Nilaway has been introduced. CodeQL Github also has been added.

Fixes: #44

@mhmxs mhmxs closed this Jan 3, 2024
@mhmxs mhmxs reopened this Jan 3, 2024
@mhmxs mhmxs force-pushed the lint-and-sec branch 3 times, most recently from ff5239b to 99cb8e2 Compare January 3, 2024 05:43
@mhmxs mhmxs closed this Jan 3, 2024
@mhmxs mhmxs reopened this Jan 3, 2024
@mhmxs mhmxs force-pushed the lint-and-sec branch 2 times, most recently from 5945030 to 4dbb85a Compare January 3, 2024 06:10
@mhmxs mhmxs closed this Jan 3, 2024
@mhmxs mhmxs reopened this Jan 3, 2024
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@mhmxs mhmxs force-pushed the lint-and-sec branch 9 times, most recently from a7e7375 to bb89744 Compare January 3, 2024 07:00
@mhmxs
Copy link
Contributor Author

mhmxs commented Jan 3, 2024

@luthermonson @avestuk @zliang-akamai Could you please take a look

Copy link
Contributor

@avestuk avestuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the SCA change. I'm glad to see the updates to break out reconcile into smaller pieces!

My one question is why you've chosen to use named returns in all of the the new reconcile functions such as reconcileUpdate. Personally I'm just not used to seeing naked returns in Go and I find it a bit jarring.

@mhmxs
Copy link
Contributor Author

mhmxs commented Jan 11, 2024

why you've chosen to use named returns in all of the the new reconcile functions

@avestuk the answer is simply because the original Reconcilie function is full of defer statements, and i didn't change the code style during refactoring.

@avestuk
Copy link
Contributor

avestuk commented Jan 11, 2024

@avestuk the answer is simply because the original Reconcilie function is full of defer statements, and i didn't change the code style during refactoring.

Eminently reasonable! What do you think about only doing named returns in Reconcile/reconcile functions where we have got the defer statements? I'm thinking that it'd be preferable to keep the named return pieces limited to places where we need them in order to modify the return values. That way it's clear why it's being done and we don't have it across the whole code base?

@mhmxs
Copy link
Contributor Author

mhmxs commented Jan 11, 2024

That way it's clear why it's being done and we don't have it across the whole code base?

@avestuk I did what i could, but reconcile update has to be an exception too.

@mhmxs mhmxs requested a review from avestuk January 11, 2024 11:44
@avestuk
Copy link
Contributor

avestuk commented Jan 11, 2024

That works for me!

@avestuk
Copy link
Contributor

avestuk commented Jan 11, 2024

reconcile update has to be an exception too.

Sorry just reading through that function and I'm not seeing why reconcileUpdate needs to be an exception?

@mhmxs
Copy link
Contributor Author

mhmxs commented Jan 11, 2024

needs to be an exception?

@avestuk Otherwise it looks like:

...() (ctr.Result, *.linodego.Instance, err) {
var res ...
var linodeInstance...
var err ...

And all the returns are the same return res, linodeInstance, err

Copy link
Contributor

@avestuk avestuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for me, but I'd prefer explicit, repeated returns than the bare return.

@mhmxs mhmxs merged commit e2af8d5 into linode:main Jan 15, 2024
5 checks passed
@mhmxs mhmxs deleted the lint-and-sec branch January 15, 2024 08:53
amold1 pushed a commit that referenced this pull request May 17, 2024
* Introduce static code analysis

* Replace named returns where possible

* Allow storage.googleapis.com for nilcheck

---------

Co-authored-by: Richard Kovacs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static code analysis during CI build
4 participants