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

GitHub Actions: add install action #5196

Merged
merged 5 commits into from
Oct 28, 2022
Merged

Conversation

mrienstra
Copy link
Contributor

@mrienstra mrienstra commented Oct 26, 2022

Changes

  • Add install action (installs pnpm, installs node, runs pnpm install)
  • To reduce duplication
  • Inspired by / based on withastro/docs .github/actions/install/action.yml
  • Has 3 optional inputs:
    • node-version: string, defaults to 16, node semvar version
    • js-runtime: string, defaults to node, installs deno if deno is provided
    • install-dependencies: boolean string, defaults to true, skips pnpm install if false (if a boolean is passed, it will be coerced into a string before reaching .github/actions/install/action.yml)

Testing

Fingers crossed!

Docs

n/a

to reduce duplication
@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2022

⚠️ No Changeset found

Latest commit: f8aaeca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the 🚨 action Modifies GitHub Actions label Oct 26, 2022
@matthewp
Copy link
Contributor

This is really interesting! I didn't know you could break up actions like this.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I love it and had no idea you could do this

@mrienstra
Copy link
Contributor Author

I already noted that this was inspired by / based on withastro/docs, but more specifically, it was @hippotastic who authored withastro/docs#431, so props to you @hippotastic! 🌟💯

Minor detail that's on my mind, looking that this again:
I went with js-runtime with the thought that it would leave room for other JS runtimes, e.g. bun (and because it "felt wrong" to have a deno-specific argument that is only used once), but arguably it would be more intuitive to change that to install-deno (boolean, default false) instead, since currently, that maps better to the behavior. js-runtime having a default of 'node' implies that node will not be installed if another value is provided, which is not the case (node is always installed).
On the other hand, not very many people need to touch Astro's GitHub Actions, so... 🤷

@mrienstra
Copy link
Contributor Author

mrienstra commented Oct 27, 2022

Whoops, I was mixing up the types for actions & workflows -- I knew only the latter accepts (and requires) type:, but I had convinced myself the former still silently accepted multiple types, which in hindsight would be pretty weird. VS Code was telling me I was wrong, but I decided that just meant I needed to open an issue and/or PR on SchemaStore/schemastore src/schemas/json/github-action.json... 🤦

Anyhoo, I got to learn a bit about JSON Schema & whatnot, and I think I've got it (this PR) all squared away now, thanks to fromJson.

@natemoo-re, you might want to take another quick peek? Everything is the same except for .github/actions/install/action.yml. Extra credit: For future reference, in this scenario (you've approved, then I make changes), your preference would be:
🚀) "Re-request review" only
🎉) @ mention
👀) "Re-request review" & @ mention
😄) None of the above

inputs:
node-version:
description: 'Node version'
required: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor detail, but just in case: required defaults to false, so these 3 lines (7, 11, 15) could be removed. I put them in with the thought that it would be helpful to state this explicitly, but alternately there could be a comment between lines 4 & 5 saying something like "all of these are optional". And/or a comment could contain a link to the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I like having the default value here, even if it is implied.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great—nice updates! Thanks for being so thorough here!

inputs:
node-version:
description: 'Node version'
required: false
Copy link
Member

Choose a reason for hiding this comment

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

I like having the default value here, even if it is implied.

@matthewp matthewp merged commit d7b27a1 into withastro:main Oct 28, 2022
matthewp added a commit that referenced this pull request Oct 28, 2022
matthewp added a commit that referenced this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 action Modifies GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants