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 CI Update #19978

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Node Version CI Update #19978

merged 5 commits into from
Apr 4, 2023

Conversation

zofskeez
Copy link
Contributor

@zofskeez zofskeez commented Apr 4, 2023

To simplify updating the node version used to build the UI, this PR updates the ci and build-vault-oss github workflows to read the node version from the ui/.nvmrc file.

@zofskeez zofskeez added this to the 1.14 milestone Apr 4, 2023
@zofskeez zofskeez requested a review from a team as a code owner April 4, 2023 16:17
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Neat! Thanks for this 🤩

- name: Set up node and yarn
uses: actions/setup-node@v3
with:
node-version: 16
node-version: ${{ steps.node-version.outputs.version}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I think we can slightly streamline this workflow by using the Node version file option.

The suggestion below means that we wouldn't need the Read node version from .nvmrc step. We use something similar when setting up Go and had no issues so far, you can see an example here.

Suggested change
node-version: ${{ steps.node-version.outputs.version}}
node-version-file: './ui/.nvmrc'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kubawi that is definitely a more streamlined option and since that option supports using package.json that's even better. Thanks for the suggestion! The test-enos-scenario-ui.yml workflow was doing something similar for both the go and node versions so I updated it there as well if you wouldn't mind having a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, looks good. I left an approval on the PR 👍

Copy link
Contributor

@kubawi kubawi left a comment

Choose a reason for hiding this comment

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

LGTM

@zofskeez zofskeez merged commit 7f3aab7 into main Apr 4, 2023
@zofskeez zofskeez deleted the ui/node-version-ci-update branch April 4, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants