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

Migrate CI to Github Actions #2931

Merged
merged 8 commits into from
Nov 12, 2020
Merged

Migrate CI to Github Actions #2931

merged 8 commits into from
Nov 12, 2020

Conversation

browniebroke
Copy link
Member

@browniebroke browniebroke commented Nov 9, 2020

Description

Migrate CI to Github Actions cc @Andrew-Chen-Wang @arnav13081994

Checklist:

  • I've made sure that tests/test_cookiecutter_generation.py is updated accordingly (especially if adding or updating a template option)
  • I've updated the documentation or confirm that my change doesn't require any updates

This doesn't aim at adding any sort of caching but just to keep feature parity with Travis for now.

Rationale

Travis recently changed their pricing with hard limits to open source projects, Github actions have a higher allowace at the moment.

Fixes #2928

@Andrew-Chen-Wang
Copy link
Contributor

It LGTM. Just regarding 46a0b60, does pydanny commit directly to master? There might be a way to remove branches: [master] and add if github.actor == pydanny (example?) to prevent pyup from triggering but still allow pydanny to commit wherever.

@luzfcb luzfcb self-requested a review November 11, 2020 03:10
@browniebroke
Copy link
Member Author

browniebroke commented Nov 11, 2020

Thanks for taking a look. I -and all core contributors- can commit directly to master as well.

@luzfcb
Copy link
Collaborator

luzfcb commented Nov 11, 2020

@browniebroke Why do we need the 46a0b60 change?
From what I understand, this will disable ci on all pull-requests...
at least it was the conclusion I got testing this pr with (luzfcb#4) and without (luzfcb#5) the 46a0b60 commit


on:
push:
branches: [ master ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
branches: [ master ]
branches: master

Copy link
Member Author

@browniebroke browniebroke Nov 11, 2020

Choose a reason for hiding this comment

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

Does this work? I though this part needs to be an array. I couldn't find an example with a string in the docs... Do you have a working example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have many working examples, but they are all private repos. Anyways this is /nit

@sfdye
Copy link
Collaborator

sfdye commented Nov 11, 2020

@browniebroke Why do we need the 46a0b60 change?
From what I understand, this will disable ci on all pull-requests...

I think that is fine. The pull requests CI will be just fine. Plus with pull_request it listens to reopen event as well.

tox:
runs-on: ubuntu-latest
strategy:
fail-fast: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

when don't we need fail fast?

Copy link
Member Author

Choose a reason for hiding this comment

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

With fail-fast the whole job would be interrupted as soon as one of the subjobs fails. In my experience it's nicer to get both to run entirely to get all failures reported as opposed to only the first failure.

- name: Tox ${{ matrix.tox-env }}
run: tox -e ${{ matrix.tox-env }}

docker:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just directly translate from travis by using matrix for docker and bare metal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, I could probably merge the docker and tox jobs, but I'm hoping that we could speed up each job leter by adding some specialised caching.

The bare metal job requires some extra services that the other jobs don't use.

@sfdye
Copy link
Collaborator

sfdye commented Nov 11, 2020

I've recently moved 20+ repos from Travis CI to Github Action for my organization, let me know if you need help with this PR

@Andrew-Chen-Wang
Copy link
Contributor

Andrew-Chen-Wang commented Nov 11, 2020

@luzfcb Pyup uses several branches itself in this repository. That would mean pyup is running the CI in a branch here and in a PR. I'm not entirely sure why the CI isn't working on your PRs, but I suspect it's because actions aren't run on forks. As you may see, this PR is built on a branch from this repo.

@browniebroke
Copy link
Member Author

@browniebroke Why do we need the 46a0b60 change?
From what I understand, this will disable ci on all pull-requests...
at least it was the conclusion I got testing this pr with (luzfcb#4) and without (luzfcb#5) the 46a0b60 commit

Hum, that's really odd. I was trying to avoid the duplicated status that you have in luzfcb#5 where each job run twice, once for the push event and once for the pull_request event. I mean, it's fine in this pull request as far as I can tell. I don't mind removing this last commit...

but I suspect it's because actions aren't run on forks

I suspect too, but again, it's strange that both run fine in the other case...

Copy link
Collaborator

@luzfcb luzfcb left a comment

Choose a reason for hiding this comment

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

LGTM

@luzfcb luzfcb merged commit 3b73295 into master Nov 12, 2020
@sfdye sfdye deleted the ci-gh-actions branch November 12, 2020 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use GitHub Actions for this instead of Travis's new pricing
4 participants