-
Notifications
You must be signed in to change notification settings - Fork 8
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
Cleanup ci and add trivy #1239
Cleanup ci and add trivy #1239
Conversation
.github/workflows/ci.yaml
Outdated
runs-on: ubuntu-22.04 | ||
timeout-minutes: 45 | ||
steps: | ||
- uses: actions/checkout@v4 |
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.
This one is missing a name
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.
LOL
.github/workflows/snap-release.yaml
Outdated
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.
Shouldn't this be moved to the actions folder like you suggested?
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.
no no, i was gonna break it into one actions but not really any point after looking at them, think composite workflow is fine .. if we break into one action we can't use "jobs" param to run up to a point as you know, it's just a single action...
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.
Why have we removed this?
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 haven't I just moved it into CI
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.
Ah it's in the CI job. I think that's a bad idea because of the pull_request_target
keyword
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.
Why do you think it's a bad idea?
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.
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.
How would moving it back protect 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.
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.
Idk if I'm understanding correctly, but if they made a pr regardless and it wanted to get secrets, doesn't seem to matter where the job is, it's just the fact you're using that trigger? So our solution is to remove the use of it?
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.
From the SO post,
This event is given repo secrets and a full read/write GITHUB_TOKEN to boot, however there is a catch - this action only runs in the pull request's target branch, and not the pull request's branch itself.
I believe this means that the CI job will not check the code from your fork, so the test will pass not because your code is necessarily correct but because the test will check out code from e.g. v3
JIMM.
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.
Not really sure what you mean? Let's move this to MM.
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 with a few comments
secrets: inherit | ||
|
||
build-and-release-jimm-server: | ||
uses: ./.github/workflows/snap-release.yaml |
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.
missing name
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.
It's a composite workflow, cannot be named
Cleanup our CI a little (renaming and whatnot) and additionally brings in govulncheck and trivy. Govulncheck appears to be necessary due to it not being added to trivy, so to cover all bases I felt it best we add it.
aquasecurity/trivy#2845
The trivy scan is currently just failing when a critical is found, but we should setup github security and have the sarifs published on each CI run. Note: On runs where we publish we will NOT be failing workflows by CI and github ecurity will be responsible for this with a security policy.
I'm currently testing govulncheck, but it's hanging, so may remove and add at a later date.