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

Move scratch-asset-types to Github Actions #16

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

delasare
Copy link
Contributor

Resolves

https://scratchfoundation.atlassian.net/browse/ENG-50

Proposed Changes

  • move from travis to github actions
  • bump to node 16 and add nvmrc file
  • create github actions workflow
  • move eslint to npm scripts
  • move tap test to npm scripts

@delasare delasare changed the title Rd/eng 50/transition scratch assets Move scratch-asset-types to Github Actions Nov 15, 2023
Copy link

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

Let me know what you think... I can imagine counter-arguments to both my suggestions :)

Comment on lines 18 to 20
npm --production=false install
npm --production=false update
npm --production=false prune

Choose a reason for hiding this comment

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

Since this has a package-lock.json now, I think this should switch to npm ci:

Suggested change
npm --production=false install
npm --production=false update
npm --production=false prune
npm --production=false ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i moved everything to dependencies rather than devDependencies since there are only 5. Eslint was failing because it could not be found. IIRC npm ci does not include devDependncies with node_env=production.

package.json Outdated
},
"scripts": {
"test": "make test"
"test": "tap ./test/unit/*.js",

Choose a reason for hiding this comment

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

Hmm... it's kind of awkward to have this out of sync with what the Makefile does. It doesn't currently matter because ./test/functional/ and ./test/integration/ don't exist, but it seems like it's setting the stage for a mistake in the future. I'd suggest either migrating everything out of Makefile and deleting it, or changing the test script back to make test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree its a little awkward. I'll have to go back and test this - I don't think Make was working and that's why I switched it. (sorry its been a while 😂 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this. Went back to Make. :)

Copy link

@skripted-io skripted-io left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@delasare delasare merged commit d1eabbd into master Dec 1, 2023
1 check passed
@delasare delasare deleted the rd/ENG-50/transition-scratch-assets branch December 1, 2023 01:35
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.

3 participants