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.

Sign up for GitHub

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

Jump to bottom

Adds generic sbt node snyk workflow #11

Merged
merged 7 commits into from Feb 9, 2022
Merged

Adds generic sbt node snyk workflow #11

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ad3553
Adds generic sbt node snyk workflow
SHession Jan 19, 2022
2d42b44
Sets Java version to 11 and simplifies node version retrieval
SHession Jan 20, 2022
165178a
Adds java_version input to the snyk workflow
SHession Feb 4, 2022
8d51820
Make org required
jorgeazevedo Feb 3, 2022
1c7bd54
Simplify handling of ORG variable
jorgeazevedo Feb 3, 2022
9b44635
Handle org whitespace
jorgeazevedo Feb 3, 2022
7c34166
rearranges cli arguments
SHession Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 37 additions & 0 deletions .github/workflows/sbt-node-snyk.yml
View file
Open in desktop
@@ -0,0 +1,37 @@
name: SBT Node Snyk
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this isn't just Node, so we might want to rename. Although I think at least in terms of clarity it might be preferable to separate the 2 actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you recommend we rename it to? My intention was to create a generic Snyk workflow which would match the general setup of our projects i.e. SBT with Node. If a project has one language then the existing Snyk actions could be used, unless we also wanted to create our own versions for reusability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misread this initially so my comment isn't really valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like Simple Snyk monitor for SBT + Node?


on:
workflow_call:
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in https://github.com/guardian/workflow-frontend/pull/356/files we're setting this up to be called on push to any branch. I think the guidance in the past has been to run synk test for branches and synk monitor only on main, so that the continuously monitored application in snyk is main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfsoul, this is a good point. Our workflows diverged from that recommendation due to failing checks against PRs which were nothing to do with the content of the PR. As many of our projects have outstanding vulnerabilities which need resolving, the test function would always provide a failing result. This was unhelpful and indicated an issue to developers unfamiliar with this process. As it represented a problem with overall codebase and not the PR specifically we felt it wasn't relevant to have as a PR check.

We therefore thought it would be preferable to exclude the test check and only run monitor, the results of which can be reviewed in the Snyk dashboard. It would be interesting to know how other teams addressed these concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussing offline I realise now that workflow-frontend will actually only be running this workflow on pushes to main - we've just commented that restriction out for testing. I do think there's value in snyk test for at least some teams, but I think this is an area that needs more thought before we can form a solid recommendation.

inputs:
DEBUG:
type: string
required: false
secrets:
SNYK_TOKEN:
required: true

jobs:
security:
runs-on: ubuntu-latest
steps:
- name: Checkout branch
uses: actions/checkout@v2

- name: Setup debug var
run: echo INPUT_DEBUG=${{ inputs.DEBUG }} >> $GITHUB_ENV
shell: bash

- uses: snyk/actions/setup@0.3.0
- uses: actions/setup-node@v2
with:
node-version-file: '.nvmrc'

- uses: actions/setup-java@v2
with:
java-version: "11"
distribution: "adopt"

- name: Snyk monitor
run: snyk monitor --all-projects ${INPUT_DEBUG:+ -d}
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't used --all-projects before, I wonder whether it'll be a bit prescriptive for more complex projects that might want to be specific about which manifests to monitor.

Relatedly, there's potentially other CLI flags we'd want to consider supporting like --org --target-reference etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all-projects has been an interesting one. As you mention it's not applicable in all cases such as flexible-content. My aim was to make a simple and generic case which would be useable across most projects. In cases where they differ, a manual workflow could be created as an alternative. My assumption is, you are trying to conceive of a way to cover the cases this wouldn't be able to cope with? If so, I think this is a good idea, but would consider this to still be a sensible starting point.

env:
SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}