Skip to content

Migrate from Travis to GitHub Action #228

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

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Conversation

keiko713
Copy link
Contributor

Several other Netlify open source repositories migrated to GitHub action already, it'd be nice to do so with open-api too.

Note: workflow is pretty much copy&paste from everywhere, any feedback is welcome.

@keiko713 keiko713 requested a review from a team as a code owner April 28, 2020 18:39
@netlify
Copy link

netlify bot commented Apr 28, 2020

Deploy preview for open-api ready!

Built with commit f2481fb

https://deploy-preview-228--open-api.netlify.app

Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks!
I can't judge for the Go part, but the Node.js part looks good 👍

Comment on lines +3 to +8
# Ensure GitHub actions are not run twice for same commits
push:
branches: [master]
tags: ['*']
pull_request:
types: [opened, synchronize, reopened]
Copy link
Contributor

Choose a reason for hiding this comment

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

on traffic mesh we are not seeing duplicated runs with just on: push: https://github.com/netlify/traffic-mesh/blob/master/.github/workflows/rust.yml#L3

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 can confess that I copy&pasted from https://github.com/netlify/js-client/blob/1000eac0fdf9434336adefdde5cce1d46c02d60e/.github/workflows/workflow.yml#L2-L8 , any opinions/suggestions @ehmicky ? (as I copy pasted from you :P)

Copy link
Contributor

Choose a reason for hiding this comment

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

This was suggested by @erezrokah and was originally copied from netlify-cms.
When doing only push, PRs that come from forked repositories do not seem to be included.
When doing both push and pull_request, PRs that come from non-forked repositories are run twice, which is the reason behind branches: [master]. The tags and types are just the default values (can be removed).

@erezrokah Please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that explains why i haven't seen it on a private repo 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice 🎉 very good to know, thanks for tips @ehmicky and @erezrokah , sounds like we could copypasta this whenever we migrate other open source repos too

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to consider is that GitHub secrets are not passed to actions triggered from forked repositories.
We had to create a separate testing flow for the CMS project:
https://github.com/netlify/netlify-cms/blob/b5a242ec8ed627e7f5e2ce8af454dd1dda1f71cc/.github/workflows/nodejs.yml#L104

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 to know, thanks for sharing @erezrokah ! I think it won't be a problem with this repo but I'll keep it in my mind 👍

keiko713 and others added 2 commits April 28, 2020 12:21
@keiko713 keiko713 merged commit bad661b into master Apr 29, 2020
@keiko713 keiko713 deleted the from-travis-to-gh-action branch April 29, 2020 00:09
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.

4 participants