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

chore(ci): move windows nodejs to choco config #2623

Merged
1 commit merged into from
Jan 25, 2022
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2022

So that it can benefit from caching.

The previous config was parameterised using parameters.node_version but this is actually more like a global constant since we don't pass in any parameters and it falls back to defaults.

Progresses #2599

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2022

Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against 749429f

@ghost ghost marked this pull request as ready for review January 25, 2022 11:39
@ghost ghost requested review from a team as code owners January 25, 2022 11:39
@ghost ghost requested review from dekelund, admons and magdziarek January 25, 2022 11:39
name: Removing pre-installed NodeJS
command: |
$current_node_version = node --version
nvm uninstall $current_node_version
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd - can't we just override the default? Or use some different image?

Copy link
Author

@ghost ghost Jan 25, 2022

Choose a reason for hiding this comment

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

Windows uses a VM image, so we can't. I tried keeping the pre-installed one but it takes precedence (nvm probably dumps itself at the start). To change that, we'd require even more brittle setup logic to change PATH ordering.

It's possible in the future once we've got the split builds and testing done, we can remove our custom NodeJS install completely and use the default one that comes with the Windows image.

To clarify, this logic existed before in a separate command, I moved it into a single command.

@@ -3,4 +3,5 @@
<package id="maven" version="3.8.2" />
<package id="gradle" version="6.8.3" />
<package id="sbt" version="1.5.5" />
<package id="nodejs" version="16.13.2" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I guess this can't be pulled from some global CI config?

Copy link
Author

@ghost ghost Jan 25, 2022

Choose a reason for hiding this comment

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

Nope. I'm thinking in a future PR, if we really wanted consistency, we can create a script to generate this file on-demand. That script can be called inside our CI config and pass through package and version parameters. Doesn't seem worth the complexity right now.

So that it can benefit from caching.
@ghost ghost force-pushed the chore/windows-nodejs-choco branch from 61ed5de to 749429f Compare January 25, 2022 12:53
@ghost ghost merged commit 8aa6f60 into master Jan 25, 2022
@ghost ghost deleted the chore/windows-nodejs-choco branch January 25, 2022 13:19
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant