Skip to content
This repository has been archived by the owner on Aug 11, 2023. It is now read-only.

Add option to use GitHub Actions for CI #449

Merged
merged 35 commits into from
Nov 18, 2020

Conversation

tepickering
Copy link
Contributor

@tepickering tepickering commented Feb 21, 2020

While not perfect, I've been finding Github Actions to be a lot easier and more convenient to use overall than Travis and Azure. Here is a basic configuration that does a matrix of python tests across OS (though only Ubuntu by default), python version, and tox environment. I used this branch to create a test repository for verifying that the Actions workflow does work. Results from a successful test run can be viewed at https://github.com/tepickering/pkgtest/actions/runs/43196684.

EDIT: Address part of actions/toolkit#499

@karllark
Copy link
Contributor

karllark commented Jun 8, 2020

Great to see github actions as potentially part of the template. I used this PR to transition one of my repositories. Very useful, especially as travis was becoming more and more cranky with time. Will be doing making the same transition for a bunch of repositories. Would be a great if this was officially part of the cookiecutter method.

@pllim pllim requested review from astrofrog and Cadair June 30, 2020 16:20
@nstarman nstarman mentioned this pull request Oct 4, 2020
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

@tepickering , I am so glad that you had the foresight to take a stab at this. Are you willing to continue to work on this? Thank you!

  • Rebase to remove conflict.
  • Remove Travis CI altogether from the template.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

p.s. The way this is set up now, maybe want to consider stages. Otherwise, maybe follow something like what is in astropy core lib to enable "fast fail". (Open to discussions.)

@tepickering
Copy link
Contributor Author

thanks for the review @pllim! now that astropy has taken the actions plunge, circling back to fix this up is high on my list. will get on it asap...

@astrofrog
Copy link
Member

I would vote with not having stages which add complication

@pllim
Copy link
Member

pllim commented Nov 16, 2020

Thanks for your patience, @tepickering ! I will be happy to have another look after the next round of changes, so feel free to ping me. 😸

@pllim
Copy link
Member

pllim commented Nov 17, 2020

the template itself is still using travis for its own CI

Yes, @Cadair said he would deal with that separately, I think.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Looking great. Thanks! Just minor comments.

TEMPLATE_CHANGES.md Outdated Show resolved Hide resolved
cookiecutter.json Outdated Show resolved Hide resolved
docs/ape17.rst Outdated Show resolved Hide resolved
docs/nextsteps.rst Outdated Show resolved Hide resolved
docs/nextsteps.rst Outdated Show resolved Hide resolved
hooks/post_gen_project.py Outdated Show resolved Hide resolved

name: CI Tests

on: [push, pull_request]
Copy link
Member

Choose a reason for hiding this comment

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

Should we also include @saimn 's patch in astropy/astropy#11054 so Actions won't also run on forks when PR is opened?

Copy link
Contributor Author

@tepickering tepickering Nov 17, 2020

Choose a reason for hiding this comment

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

yes, this keeps the template in-line with what astropy is doing so definitely worth including.

@pllim pllim mentioned this pull request Nov 17, 2020
2 tasks
@nstarman
Copy link
Member

The old travis setup split the tests into sections: "initial" and "comprehensive". I recently switched to GitHub CI and have found this to be possible by splitting the tests into jobs, setting fail-fast=False within each job and setting up job dependencies.

As an example,

ci_tests.txt

@pllim
Copy link
Member

pllim commented Nov 17, 2020

sections: "initial" and "comprehensive"

@astrofrog already stated his objection to re-introducing stages in Actions due to increased complexity.

@nstarman
Copy link
Member

nstarman commented Nov 17, 2020

increased complexity.

Is there any other means to prevent a code linter check from stopping the other tests?
the codestyle is useful, but ...

# https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners
ci_tests:
name: ${{ matrix.name }}
runs-on: ${{ matrix.os }}
Copy link
Member

@nstarman nstarman Nov 17, 2020

Choose a reason for hiding this comment

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

Someone kindly gave me this piece of code to allow for skipping CI. Passing it forward.

if: "!(contains(github.event.head_commit.message, '[skip ci]') || contains(github.event.head_commit.message, '[ci skip]'))"

Copy link
Member

@pllim pllim Nov 17, 2020

Choose a reason for hiding this comment

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

Does that still work? It didn't work for me when I tried at astropy/astropy#11044 . In fact, I dumped the whole github.event context out and couldn't find any head_commit in it.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested it recently. It worked when I first added it and doesn't NOT work. But I see 11044 is more recent than my last test, so I guess it doesn't work anymore 👎 .

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 had this in the template originally, but took it out because it's no longer a reliable method of doing this. i would prefer to leave this alone until either it's supported natively (actions/runner#774) or there's an official action published that will do this properly. a 3rd party action exists, https://github.com/marketplace/actions/ci-skip-action, and seems to work well. astropy may wish to make their own version of this if native support is not forthcoming.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this worked for me in photutils when I tested it:
https://github.com/astropy/photutils/blob/master/.github/workflows/ci_tests.yml#L18

Copy link
Member

Choose a reason for hiding this comment

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

Nope, no work astropy/photutils#1121

branches:
- master
tags:
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

Might be too much of an edge case, but adding

  pull_request:
    branches:  # only build on PRs against main
      - master

likewise limits PR builds. I have this set on my private repos to preserve minutes (public too if I forgot to comment it out).

Copy link
Member

Choose a reason for hiding this comment

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

Probably too specific to be useful. Most repos don't do branch and for those that do, they probably want to test even if a PR is opened against older release branch (especially if they are following gitflow).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this actually reminds me that we should be changing master here to main which is GitHub's new default...

@pllim
Copy link
Member

pllim commented Nov 17, 2020

Is there any other means to prevent a code linter check from stopping the other tests?

Without stages and with "fail fast" on (the default), you hope that it finishes first, or you make it into another job. But I think in some cases, you actually want a failing linter check to stop other jobs. 🤷

@tepickering
Copy link
Contributor Author

i think in the use case of starting from scratch with a fresh code base, having the linter check fail fast and stop everything else is useful. shimming an actions workflow into an existing codebase is a different use case where you either want to allow the check to fail somehow or adjust the check to only fail in the worst cases.

@tepickering
Copy link
Contributor Author

i built a dummy repo from this branch and you can see the results of the CI here:

https://github.com/tepickering/tep_test/actions/runs/369174337

a bit of light whingeing about ubuntu-latest moving to ubuntu-20.04 soon, but clean otherwise.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Just some wording nitpicks, otherwise LGTM. Thank you so much!

QUESTIONS.rst Outdated Show resolved Hide resolved
TEMPLATE_CHANGES.md Outdated Show resolved Hide resolved
TEMPLATE_CHANGES.md Outdated Show resolved Hide resolved
docs/ape17.rst Outdated Show resolved Hide resolved
docs/nextsteps.rst Outdated Show resolved Hide resolved
docs/nextsteps.rst Outdated Show resolved Hide resolved
docs/nextsteps.rst Outdated Show resolved Hide resolved
docs/options.rst Outdated Show resolved Hide resolved
@pllim pllim merged commit 589d874 into astropy:cookiecutter Nov 18, 2020
@pllim
Copy link
Member

pllim commented Nov 18, 2020

Thanks, @tepickering and everyone who reviewed!

@tepickering tepickering deleted the add_github_actions branch November 18, 2020 02:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants