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

[snyk workflow] make path to node version file configurable #12

Merged
merged 1 commit into from
Feb 25, 2022
Merged

[snyk workflow] make path to node version file configurable #12

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion .github/workflows/sbt-node-snyk.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ on:
type: string
required: false
default: "11"
NODE_VERSION_FILE:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've followed the existing convention here (MY_INPUT), but in my experience GitHub Actions inputs are typically defined as my-input (see examples) - I wonder if we should make that change now before too many people pick this up.

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we're able to use different node versions within the same repo? To provide a simple example against this: we've seen a few lambdas with a node 12 runtime, but build with node 14. That's pretty confusing IMO.

Are there any reasons one would want to use a different node version within a single repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess there are a few reasons that we might want to specify the path to the node version file rather than using a hardcoded default:

  1. The repo uses a single node version (defined in .nvmrc), but the file is not at the project root (this is my use case - cdk projects typically seem to include this file under /cdk).
  2. The repo uses a single node version, but it uses .node-version (rather than .nvmrc) to manage the node version.
  3. The repo uses multiple node versions e.g. version A for cdk and version B for application code. I'm not 100% sure if this is desirable/acceptable and I'm not convinced it'd work properly with this workflow!

Copy link
Member

Choose a reason for hiding this comment

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

  1. The repo uses a single node version (defined in .nvmrc), but the file is not at the project root (this is my use case - cdk projects typically seem to include this file under /cdk).

I think GuCDK is wrong here - it should place this file at the root (possibly tricky if it already exists).

  1. The repo uses a single node version, but it uses .node-version (rather than .nvmrc) to manage the node version.

Not sure we do this (rough GH search), but a fair point.

  1. The repo uses multiple node versions e.g. version A for cdk and version B for application code. I'm not 100% sure if this is desirable/acceptable and I'm not convinced it'd work properly with this workflow!

See 1. I think we should do everything we can to avoid this as it's pretty confusing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merged this PR as I think it's still sensible to allow users to configure this if it's necessary, but I agree with:

it should place this file at the root (possibly tricky if it already exists).

so I'll see if it can be easily moved for AMIgo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've followed the existing convention here (MY_INPUT), but in my experience GitHub Actions inputs are typically defined as my-input (see examples) - I wonder if we should make that change now before too many people pick this up.

Definitely think we should do this sooner rather than later!

type: string
required: false
default: ".nvmrc"
secrets:
SNYK_TOKEN:
required: true
Expand All @@ -27,7 +31,7 @@ jobs:
- uses: snyk/actions/setup@0.3.0
- uses: actions/setup-node@v2
with:
node-version-file: '.nvmrc'
node-version-file: ${{ inputs.NODE_VERSION_FILE }}

- uses: actions/setup-java@v2
with:
Expand Down