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

Add ability to use package json version. #469

Closed
wants to merge 1 commit into from
Closed

Add ability to use package json version. #469

wants to merge 1 commit into from

Conversation

The-Code-Monkey
Copy link

This will only work with top level package json file and not nested files.

Description:
Adds the ability to specify "package" as node version instead of a number

Related issue:
#467

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

This will only work with top level package json file and not nested files.
@The-Code-Monkey The-Code-Monkey requested a review from a team April 19, 2022 08:52
const packageJson = JSON.parse(
fs.readFileSync(
path.join(
process.env.GITHUB_WORKSPACE!,
Copy link

Choose a reason for hiding this comment

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

Suggested change
process.env.GITHUB_WORKSPACE!,
process.env.GITHUB_WORKSPACE,

I think we should use node-version-file or similar here to support package.json in subdirectory.

Besides, is the version definition format in package.json (it includes eg. version ranges) the same as expected here or we have to resolve it?

Copy link
Author

Choose a reason for hiding this comment

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

The idea behind this is for people who have a fixed node version. So doesn't need to interpret anything other than the value. Also I copied the code from above in the file so don't think it requires changing

@e-korolevskii
Copy link
Contributor

Hi, @The-Code-Monkey,

Unfortunately checks were not successful. First of all you need to manually run build and add the results to the PR.

Cheers.

@The-Code-Monkey
Copy link
Author

Ok no worries I've not had much time to get back to the pr and fix it been busy with work

@e-korolevskii
Copy link
Contributor

Hi @The-Code-Monkey,

Just a gentle ping.

@brcrista
Copy link
Contributor

This is addressing the same issue as #485, correct? Seems like that one is farther along, so let's go with that one.

@brcrista brcrista closed this Jul 21, 2022
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