Silence govulncheck

[MDr164]

As we only pin the minor version and not the patch version of Go (as most projects do), we can't guarantee the CI to use a toolchain version which has security fixes govulncheck would otherwise complain about.

EDIT: We keep the go.mod as is but modified the CI pipeline as outlined below.

[rdimitrov]

I recall we had that at some point and the problem is that this enforce other projects that use go-tuf to also pin the minor version and not everyone likes that.

To be honest I'm in favour of removing govulncheck or make failing the job not failing the whole workflow just because we can't silence this.

[MDr164]

I'm also not a fan of having a patch version pinned in the go.mod file but I also didn't want to turn off any CI checks you guys implemented. I think we could keep it but have it as part of a CVE run that combines govulncheck and codeql maybe? This way we could have it not turn red on every single CI run I suppose.

[rdimitrov]

Yeah, exactly 👍 Let's stay away from pinning this. Also I think what you suggest is reasonable 👍

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.96%. Comparing base (14cf073) to head (2fd1859).
Report is 20 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   70.51%   72.96%   +2.45%     
==========================================
  Files          10       10              
  Lines        2123     1650     -473     
==========================================
- Hits         1497     1204     -293     
+ Misses        505      325     -180     
  Partials      121      121              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MDr164]

Ok setting it to always pass works but this feels wrong as well. Now this check is always green even if it found issues. You can still click on the job itself to look at the report but the overview is green. I think we should add a more fine tuned linter config that marks common security pitfalls as CI fails (e.g. enable gosec in golangci-lint) instead of relying on the CVE checker here. Though in the current state it at least doesn't mark CI runs as failed...

[rdimitrov]

What do you think about adding a bit of automation around this? If it sounds dumb feel free to say it's dumb, no worries 😄 I'm thinking we can keep the continue-on-error part on the job, but add another step at the end that checks the status of the govulncheck step. If it's a failure comment on the PR that triggered the workflow run saying something like "Hey, govulncheck found something, make sure to check it". I haven't tried it (I can though if you say it makes sense), so I'm not sure if it will work, but it sounds feasible I think.

[kommendorkapten]

I think we should have continue-on-error. As this is a module (and a source distribution) and it's ultimately any client that compiles the code with a specific compiler, I don't see much we can do for bugs in Go.

As discussed we could bin the minor, but I believe that is something a module should not do, it's the responsibility of the project that uses this module to define what minor version to use.

[MDr164]

What do you think about adding a bit of automation around this? If it sounds dumb feel free to say it's dumb, no worries 😄 I'm thinking we can keep the continue-on-error part on the job, but add another step at the end that checks the status of the govulncheck step. If it's a failure comment on the PR that triggered the workflow run saying something like "Hey, govulncheck found something, make sure to check it". I haven't tried it (I can though if you say it makes sense), so I'm not sure if it will work, but it sounds feasible I think.

That's definitely feasible and only requires another step after govulncheck to run conditionally which is fully supported. I'll have a go at it on a test repo first to not spam this PR. I think the only 'tricky' part is to not have it spam on PR updates, aka behave like the Codecov comment.

[rdimitrov]

Sounds good 👍 Alright, let's merge this then and we can try to make its output more explicit in another one 👍

[MDr164]

Feel free to merge, I still don't have the necessary repo rights to do so 😅