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

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

Merged
merged 1 commit into from Feb 25, 2022

Conversation

jacobwinch
Copy link
Contributor

What does this change?

Follow-up to #11. In order to use this in my project I need to be able to configure the path to the .nvmrc file.

How to test

I've tested this with Amigo: guardian/amigo#698.

How can we measure success?

More projects can benefit from this shared workflow.

Have we considered potential risks?

I don't think this PR introduces any new risks.

@@ -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!

@jacobwinch jacobwinch changed the title [snyk workflow] make path to .nvmrc file configurable [snyk workflow] make path to node version file configurable Feb 21, 2022
@jacobwinch jacobwinch merged commit cc9efbd into main Feb 25, 2022
@jacobwinch jacobwinch deleted the jw-configurable-nvmrc-path branch February 25, 2022 09:42
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.

None yet

3 participants