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

ci: move format, lint and test jobs to GitHub Actions #1814

Merged
merged 21 commits into from Feb 9, 2020

Conversation

merlinnot
Copy link
Contributor

@merlinnot merlinnot commented Nov 8, 2019

This does the following:

  • Check formatting using latest Ubuntu, Node.js 12
  • Run ESLint using latest Ubuntu, Node.js 12
  • Run tests across operating systems and Node.js versions

Some trickery is used to cache ESLint results. npm cache is mostly straightforward, with a one workaround for Windows. Both explained in comments.

I renamed prettier step to format. Why? Because it's easier to understand for contributors. There are multiple formatters available, not everyone knows what Prettier is (I personally love it and use all the time, but we should be inclusive). Name "format" is more understandable. Same for lint:fix and format:fix -> no surprises, self-explanatory. Hope you'll like the change.

You can see it in action here, I made a PR on my branch: https://github.com/merlinnot/nock/pull/1/checks

GitHub Actions go GA in five days. To start running it today, you need to enable it for the repo here: https://github.com/features/actions (open for everyone).

For the last two steps I'll create separate PRs.

Resolves #1792

package.json Show resolved Hide resolved
Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

This looks awesome to me! I left a few small comments and questions.

I’m not exactly sure how to on Actions for the repo or what is needed to make this run. It’s already turned on for the org 🤔

.github/workflows/continuous-integration.yaml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yaml Outdated Show resolved Hide resolved
@merlinnot
Copy link
Contributor Author

I’m not exactly sure how to on Actions for the repo or what is needed to make this run. It’s already turned on for the org 🤔

Then I guess it should work once we merge it 🤔 I think we can merge it and test out, I can't find precise docs on this. We can always revert if it doesn't work for some reason 😄

Copy link
Member

@mastermatt mastermatt left a comment

Choose a reason for hiding this comment

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

I don't have much experience with Actions, but I'm a fan of the idea

@merlinnot
Copy link
Contributor Author

@paulmelnikow Shall we get it merged?

@paulmelnikow
Copy link
Member

It seems like perhaps there isn't a way to run this on the nock repo until this gets merged so I'd like to go ahead and do that.

One concern I have is if tests are no longer run in Travis, the semantic-release pipeline will not be dependent on tests. Do you think you could update the Travis config to run tests on master before releasing it? I think @gr2m has done a lot of work on getting semantic-release to run in Actions so I think it'll be possible to do that, though that isn't blocking and this has been open a long time.

@gr2m
Copy link
Member

gr2m commented Feb 5, 2020

One concern I have is if tests are no longer run in Travis, the semantic-release pipeline will not be dependent on tests

Actually I think that is a good think. We have enforced the pull request workflow, nobody should push to master directly. With that, we don't need to run tests on master, because nothing can be merged without the green check from the tests. Once merged, semantic-release should just do its thing.

It's super simple to run semantic-release from Actions, too. I do it all the time.

Here is an example setup of mine

For the release flow, we need to create add the NPM_TOKEN secret to the repo

We can do a follow up PR after we merge this to move the relaese to Actions, too.

@@ -25,24 +23,9 @@ jobs:
node_js: lts/*
script:
- git checkout $TRAVIS_BRANCH
- npm run prettier
- npm run format:fix
# commit changes and push back to branch on GitHub. If there are no changes then exit without error
- 'git commit -a -m "style: prettier" && git push "https://${GH_TOKEN}@github.com/$TRAVIS_REPO_SLUG" ${TRAVIS_BRANCH} || true'
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we can. Although I'd prefer to do it in a separate PR instead of making this one even larger. Is it ok with you? Seems like it doesn't have to be merged at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely, I'd suggest the same. Sorry, I should have said so

@merlinnot
Copy link
Contributor Author

@paulmelnikow I understand that since "branch up to date" requirement is enabled, you're ok with merging it as-is? I think it's good to have this setting enabled, so the master branch is always healthy.

@paulmelnikow
Copy link
Member

Yup, it looks good to me! @gr2m does this look good to you?

@gr2m gr2m mentioned this pull request Feb 6, 2020
Closed
@gr2m
Copy link
Member

gr2m commented Feb 6, 2020

moved to #1870 to see how the jobs run

@gr2m
Copy link
Member

gr2m commented Feb 7, 2020

@merlinnot any reason why you put all the jobs in a single workflow? I would prefer to split them up into multiple *.yml workflow files. It would also allow us to run the tests on greenkeeper/** branches, the same is not necessary for the linting

@merlinnot
Copy link
Contributor Author

Yes, there are actually some reasons to do so. For Greenkeeper itself I see two

  • ability to use Merge me! Action to automatically merge pull requests from Greenkeeper when the CI passes (https://github.com/ridedott/merge-me-action)
  • Greenkeeper updates also include updates to the linting and formatting tools themselves, which might cause the branch not to pass cleanly anymore (eg. when new rules or fixes are introduced). That should be caught before merging to master, so I'd argue that lint and format tasks should run for dependency updates too.

@mastermatt
Copy link
Member

so I'd argue that lint and format tasks should run for dependency updates too.

I agree with this.

@gr2m
Copy link
Member

gr2m commented Feb 9, 2020

Greenkeeper updates also include updates to the linting and formatting tools themselves, which might cause the branch not to pass cleanly anymore (eg. when new rules or fixes are introduced). That should be caught before merging to master, so I'd argue that lint and format tasks should run for dependency updates too.

I would create separate actions that run lint:fix / format:fix and commits the changes back to pull requests coming from Greenkeeper for prettier an eslint, like this: https://github.com/octokit/core.js/blob/master/.github/workflows/update-prettier.yml

That will have the benefit that the PRs will fix themselves, we won't need to run the commands and push back the changes manually

@merlinnot
Copy link
Contributor Author

For the updates to be pushed back to the branch it doesn't really matter if it's a single file or multiple files, right? While I personally don't mind doing or not doing it (both have pros and cons), I committed to implementing it somewhere above, it's just that I'd like to do it in a separate PR, so that it doesn't grow too much. Small steps. Is that ok with you, or do you feel the need to have this feature in this particular PR?

@merlinnot
Copy link
Contributor Author

Oh, and you can see this particular branch being built here: merlinnot#1

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

All sounds good to me. Great work @merlinnot 💐

@gr2m gr2m merged commit e863c84 into nock:master Feb 9, 2020
merlinnot added a commit to merlinnot/nock that referenced this pull request Feb 9, 2020
@nockbot
Copy link
Collaborator

nockbot commented Feb 10, 2020

🎉 This PR is included in version 11.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speeding up our builds
6 participants