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

feat(ci): add a validation step for starters #10696

Merged
merged 8 commits into from
Jan 4, 2019

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Dec 28, 2018

Description

This PR will (start) to add some increased validation and checks for starters. In particular, it will generate a yarn.lock file in each starter (and add this to the read-only clone), as well as run the build script to validate a successful build.

During a pull request, it will:

  • Run npm install
  • Run npm run build

This will catch issues with a dependency change causing a failure, but it will not (currently) catch something like a failure introduced in the current changes in the PR (e.g. a change to Gatsby like in #10593). Fixing both of these are worth exploring, I just don't want to slow down the build too much.

During a merge to master, it will:

  • Run npm install
  • Run npm run build
  • Run yarn import (generate a yarn.lock file based on the contents of package-lock.json)
  • Sync to gatsbyjs/gatsby-starter-NAME

My main concern with the merge to master is that it's possible to fail and not know it fails. We do merge to master enough that it's not a huge concern and we'd catch it the next pull request, but worth noting.

Related Issues

Begins to address #10607

@DSchau DSchau requested a review from a team as a code owner December 28, 2018 02:38
@DSchau
Copy link
Contributor Author

DSchau commented Dec 28, 2018

Also - I want to think more on this approach.

This doesn’t seem extremely scalable, and I want to avoid having to resort to editing shell scripts and adding additional (possibly slow) checks for every new functionality we want to validate.

It’ll work—but I’d like to see if there’s something cleaner and perhaps simpler.

@DSchau DSchau removed the status: WIP label Jan 1, 2019
@DSchau
Copy link
Contributor Author

DSchau commented Jan 3, 2019

So after thinking more on this, it may make sense to just make starters part of lerna/workspaces—then we can run whatever arbitrary tasks we want in the repo pretty cleanly.

@pieh what this PR has works, but could possibly be improved. What do you think?

@pieh
Copy link
Contributor

pieh commented Jan 3, 2019

I think this looks good to test. One thing that potentially worries me is the lock generation: If I read this right - it will (re)generate lock files on master commits. There is slight problem - lerna pushes publish commits before it push packages to npm registry, so we might be "version" behind with the lock files if we install deps before publish is complete?

@DSchau
Copy link
Contributor Author

DSchau commented Jan 3, 2019

@pieh your comment there is if we swap to the lerna/workspaces approach, correct? As it works now, we're just basing the yarn.lock generation off of what exists in package-lock.json (which currently isn't auto-generated whatsoever, unfortunately).

@DSchau DSchau changed the title [WIP]: ci: add a validation step for starters feat(ci): add a validation step for starters Jan 4, 2019
Copy link
Contributor

@pieh pieh 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, let's take it for test drive

@pieh pieh merged commit 253580a into gatsbyjs:master Jan 4, 2019
@DSchau DSchau deleted the starters/validate branch January 4, 2019 18:55
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
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.

2 participants