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

ci: Adding gosec in golangci lint #32

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

subhamkrai
Copy link
Contributor

@subhamkrai subhamkrai commented Jul 4, 2024

Describe what this PR does

ci: add gosec in golangci lint check

adding gosec in golangci lint check, also skip gosec G204 as we don't
want to run gosec on exec method

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@subhamkrai subhamkrai force-pushed the add-gosec-golangci branch 2 times, most recently from d26095a to 5b92fc6 Compare July 4, 2024 12:28
@subhamkrai subhamkrai changed the title ci: Adding kubsec CI and gosec in golangci ci: Adding gosec in golangci lint Jul 4, 2024
@subhamkrai subhamkrai marked this pull request as ready for review July 5, 2024 03:49
@nb-ohad
Copy link
Collaborator

nb-ohad commented Jul 5, 2024

@subhamkrai Can you please expend on the reason to disable G204?

@subhamkrai
Copy link
Contributor Author

@subhamkrai Can you please expend on the reason to disable G204?

@nb-ohad gosec G204 gives error when subprocess is started with in the a process, like exec.Command and in other repos also upstream I have seen they suggesting to either disable gosec on that line or disable G204

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 8, 2024

@subhamkrai we should not disable the security rules in the whole repo, if required disable it by adding a comment where it is used or disable it in the test files (if possible)

@subhamkrai
Copy link
Contributor Author

@subhamkrai we should not disable the security rules in the whole repo, if required disable it by adding a comment where it is used or disable it in the test files (if possible)

we can disable for single line, but this rule was something I noticed disabled most places

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 8, 2024

@subhamkrai we should not disable the security rules in the whole repo, if required disable it by adding a comment where it is used or disable it in the test files (if possible)

we can disable for single line, but this rule was something I noticed disabled most places

these kind of rules need to be verified and disable on demand based on the impact, For now lets not disable it by default rather disable it by putting a comment on the line in code

@subhamkrai subhamkrai force-pushed the add-gosec-golangci branch 2 times, most recently from 86999a0 to 9a1f788 Compare July 8, 2024 12:03
@subhamkrai
Copy link
Contributor Author

@subhamkrai we should not disable the security rules in the whole repo, if required disable it by adding a comment where it is used or disable it in the test files (if possible)

we can disable for single line, but this rule was something I noticed disabled most places

these kind of rules need to be verified and disable on demand based on the impact, For now lets not disable it by default rather disable it by putting a comment on the line in code

there is small problem now, we need to put //nolint:gosec in the same line as exec.command but when exec.command is spilt into multiple lines is failing with G204 but if we keep exec.command golangci lint in same long line it will fail again due to char limit per line again golangci lint is failing

@subhamkrai
Copy link
Contributor Author

@subhamkrai we should not disable the security rules in the whole repo, if required disable it by adding a comment where it is used or disable it in the test files (if possible)

we can disable for single line, but this rule was something I noticed disabled most places

these kind of rules need to be verified and disable on demand based on the impact, For now lets not disable it by default rather disable it by putting a comment on the line in code

there is small problem now, we need to put //nolint:gosec in the same line as exec.command but when exec.command is spilt into multiple lines is failing with G204 but if we keep exec.command golangci lint in same long line it will fail again due to char limit per line again golangci lint is failing

@Madhu-1 ^

@nb-ohad
Copy link
Collaborator

nb-ohad commented Jul 8, 2024

@subhamkrai Where exactly do we have exec.command in the repo? We should not have it in code that runs on production. Do we have something like that in tests or supporting code?

test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@nb-ohad
Copy link
Collaborator

nb-ohad commented Jul 9, 2024

@subhamkrai Can you please fix the lint errors?

@subhamkrai
Copy link
Contributor Author

@subhamkrai Can you please fix the lint errors?

@nb-ohad misspell error will go away, and for golangCI lint we need to decided how we want to fix as I mentioned

there is small problem now, we need to put //nolint:gosec in the same line as exec.command but when exec.command is spilt into multiple lines is failing with G204 but if we keep exec.command golangci lint in same long line it will fail again due to char limit per line again golangci lint is failing

in above comment

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 9, 2024

@subhamkrai did you try to add that comment on top of that line?

test/utils/utils.go Outdated Show resolved Hide resolved
adding gosec in golangci lint check, also skip gosec G204 as we don't
want to run gosec on exec method.

Signed-off-by: subhamkrai <[email protected]>
@nb-ohad nb-ohad merged commit b6e739d into ceph:main Jul 9, 2024
6 checks passed
@subhamkrai subhamkrai deleted the add-gosec-golangci branch July 9, 2024 09:35
leelavg pushed a commit to leelavg/ceph-csi-operator that referenced this pull request Oct 4, 2024
Bug 2313203:[release-4.17] add missing rbac for persistentvolume during deletion
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.

3 participants