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

Setting Node version with nvm under machine images [semver:patch] #73

Merged
merged 5 commits into from
Mar 29, 2021

Conversation

felicianotech
Copy link
Contributor

This should be rebased, re-run (and should pass) after #72 is merged.

@felicianotech felicianotech requested a review from a team February 18, 2021 23:53
@dsayling
Copy link
Contributor

dsayling commented Feb 19, 2021

any reason to not include this change in #72 ?

@felicianotech
Copy link
Contributor Author

Just PR etiquette that I like to follow. It's good practice.

For example, Gabe wasn't even sure if we should add this test. This allows us to easily ignore or revert this change without touching #72.

@dsayling
Copy link
Contributor

Just PR etiquette that I like to follow. It's good practice.

For example, Gabe wasn't even sure if we should add this test. This allows us to easily ignore or revert this change without touching #72.

I agree with keeping the changes small, but since this test validates the change in #72, IMHO, I would include it.

@KyleTryon
Copy link
Contributor

I also see the etiquette but feel as though it makes more sense to combine these. Though as the other PR shouldnt have any negative effects, I could take this either way. But as these two changes are related to the same single issue, I'd vote to combine.

@dsayling
Copy link
Contributor

@felicianotech looks like #72 did not resolve #62

@gmemstr
Copy link
Contributor

gmemstr commented Feb 22, 2021

I'm not 100% on why this is yet but it looks like the nvm version isn't being correctly set in the environment for whatever reason, despite installing fine.

@gmemstr gmemstr mentioned this pull request Feb 26, 2021
@felicianotech
Copy link
Contributor Author

@gmemstr @dsayling Can either of you point me to an example of where there is still an issue with the Node.js orb v4.2.0 doesn't work right?

@felicianotech
Copy link
Contributor Author

I'm still learning how the embedded scripts work. Where does this grep error come from? I don't see where we use it. https://app.circleci.com/pipelines/github/CircleCI-Public/node-orb/687/workflows/4214ce53-c75d-4673-8aeb-a3aec12f1f95/jobs/4459

This will have to be readded once new machine image version have rolled.
@KyleTryon KyleTryon changed the title Test with machine image [semver:skip] Test with machine image [semver:patch] Mar 29, 2021
This will have to be readded once new machine image version have rolled.
@gmemstr gmemstr changed the title Test with machine image [semver:patch] Allow setting Node version with nvm under machine images [semver:patch] Mar 29, 2021
@gmemstr gmemstr changed the title Allow setting Node version with nvm under machine images [semver:patch] Setting Node version with nvm under machine images [semver:patch] Mar 29, 2021
@gmemstr gmemstr merged commit 2647831 into master Mar 29, 2021
@gmemstr gmemstr deleted the test-machine branch March 29, 2021 16:01
@gmemstr gmemstr mentioned this pull request Mar 30, 2021
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

4 participants