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

node-version as fallback of node-version-file #939

Open
y-nk opened this issue Jan 10, 2024 · 8 comments
Open

node-version as fallback of node-version-file #939

y-nk opened this issue Jan 10, 2024 · 8 comments
Labels
feature request New feature or request to improve the current logic

Comments

@y-nk
Copy link

y-nk commented Jan 10, 2024

Description

Currently the behavior is that node-version is preferred rather than node-version-file if both are provided.

setup-node/src/main.ts

Lines 85 to 95 in d86ebcd

if (version && versionFileInput) {
core.warning(
'Both node-version and node-version-file inputs are specified, only node-version will be used'
);
}
if (version) {
return version;
}
if (versionFileInput) {

I would like to propose the opposite since node-version is actually hardcoded in the action parameters, but the file is not guaranteed to be there.

setup-node/src/util.ts

Lines 8 to 12 in d86ebcd

if (!fs.existsSync(versionFilePath)) {
throw new Error(
`The specified node version file at: ${versionFilePath} does not exist`
);
}

Currently, scenarios resolved as following:

with:
  node-version: 8 # nothing to see here
with:
  node-version-file: fileExists.json # all good
with:
  node-version-file: fileDoesNotExists.json # throws error and crashes the pipeline
with:
  node-version: 8
  node-version-file: fileExists.json # ignored, picked version is 8
with:
  node-version: 8
  node-version-file: fileDoesNotExists.json # ignored, picked version is 8

What I'm requesting for is a change in the 2 lasts scenarios:

with:
  node-version: 8
  node-version-file: fileExists.json # has priority, so it's picked
with:
  node-version: 8
  node-version-file: fileDoesNotExists.json # does not throw and picks version 8 instead

Justification

This is just a nicer API, i think. it does not remove the case of conscious crash of the pipeline if no node-version is given, but it does nicely propose a fail-safe for people interested.

Note

We could also have another parameter such as node-version-fallback-if-file-not-found but it'd just increase the complexity of the api.

Are you willing to submit a PR?

Yes, i'm ok with that.

@y-nk y-nk added feature request New feature or request to improve the current logic needs triage labels Jan 10, 2024
@aparnajyothi-y
Copy link
Contributor

Hello @y-nk
Thank you for creating this issue, we will investigate it and come back to you as soon as we have some feedback.

@fregante
Copy link

fregante commented Feb 8, 2024

To me this feels backwards; generally, config files are the default and "inline settings/flags" override the defaults. If you're specifying a node-version, then you most likely prefer that one over the "project-level" version defined in the package.json

@y-nk
Copy link
Author

y-nk commented Apr 18, 2024

@fregante that is untrue, especially in monorepos where the node-version file can be there (or not) depending on the package/service running the job, but you could always use a hardcoded base version as a fallback.

@fregante
Copy link

fregante commented Apr 18, 2024

I don’t think "monorepos" are the standard. I'm thinking more about CLIs and literally everything else:

  1. Use call flag
  2. Use local config
  3. Use global config
  4. Use default

I don’t think it should be anything but that.

@y-nk
Copy link
Author

y-nk commented Apr 24, 2024

@fregante i understand your standpoint. as a mean not to remain unblocked, would you have any recommendation for something less verbose/more elegant than this:

    - if: ${{ !inputs.workspaceRelativePath }}
      uses: actions/setup-node@v4
      with:
        node-version: 20
        cache: 'pnpm'
        registry-url: https://npm.pkg.github.com/
        scope: '@company'

    - if: ${{ inputs.workspaceRelativePath }}
      uses: actions/setup-node@v4
      with:
        node-version-file: ${{ format('{0}/.node-version', inputs.workspaceRelativePath) }}
        cache: 'pnpm'
        registry-url: https://npm.pkg.github.com/
        scope: '@company'

?

@mk-pmb
Copy link

mk-pmb commented Jun 12, 2024

I agree with @fregante in that I expect my explicitly given parameter in the CI workflow file to have more weight than the files in the repo. My CI workflow is there to keep repo file changes in check. Maybe we can have a weak option fallback-node-version; albeit I'd prefer combining all of the possibilities into just node-version as a priority list. You could then specify something like >=16; file:package.json; file:.nvmrc; ; 0; 18; 20 that works like this:

  • >=16 sets a mandatory requirement. If our decision would conflict with that, it's a fatal error.
  • file:package.json if possible, decide by this file and ignore further entries.
  • file:.nvmrc if possible, decide by this file and ignore further entries.
  • (empty) ignore. The calling workflow probably had a variable insertion here that happened to not be used in this case.
  • 0: Similar to empty but probably as result of a math operation with empty inputs. For convenience we should ignore literal 0; if someone really wants 0.x they can add the .x or prepend a =.
  • 18 a literal version. Since this decision mechanism will always succeed, the list will effectively end here.
  • 20 probably an accident since this can never be reached. We could consider warning users about this, or silently ignore it because the previous 18 may have been from an optional parameter like the empty slot was.

Update: I don't fully understand the PATH problem in #912, but it seems to in some way be a version number oracle, so we shoudl probably support it here as well. Name could be path or existing (not sure if I understood the intent enough).

@ssbarnea
Copy link

I have an issue closely related to this that is related to use of multiple node version

$ cat .tool-versions 
nodejs 20.18.0 18.20.4

Basically the .tool-versions file defines two different node versions that are supported by the project, with 20 being the implicit one.

Currently it seems impossible to use actions/setup-node to create a test matrix that uses these two versions as there is no argument that I pass to tell it to use one or another.

This means that if I want to test both, I have to copy the exact version from the .tool-versions into all the GHA workflows that are running with both versions and also to find a magic way to keep this in sync, so they will not diverge.

Does anyone have a workaround for this issue?

@y-nk
Copy link
Author

y-nk commented Nov 19, 2024

@ssbarnea you need to create a first job which will read the .tool-versions and build a json out of it (using job outputs), then pass that json to a 2nd job into your matrix.

job1:
  runs-on: ubuntu-latest
  outputs:
    node-versions: ${{ steps.tool-versions.outputs.result }}

  steps:
    - id: tool-versions
      run: |
        # this bash was suggested by chatgpt.
        result=$(cat .tool-versions | jq -R 'split(" ") | { (.[0]): .[1:] } | .nodejs')
        echo "result=$result" >> $GITHUB_OUTPUT

job2:
  needs: job1
  runs-on: ubuntu-latest
  strategy:
    matrix:
      node-version: ${{ fromJSON(needs.job1.outputs.node-versions) }}
  steps:
    - uses: actions/setup-node@v5
      node-version: ${{ matrix.node-version }}

drafted hastily so some typos may occur ; general idea is there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

No branches or pull requests

5 participants