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 to GitHub actions #304

Merged
merged 13 commits into from
Jan 20, 2022
Merged

Migrate to GitHub actions #304

merged 13 commits into from
Jan 20, 2022

Conversation

scasagrande
Copy link
Contributor

@scasagrande scasagrande commented Dec 18, 2020

Better late than never?

PR swaps the repo over to using Github actions instead of Travis. This should maintain the same test coverage, reporting, artifact uploading on releases, and pylint.

I did however turn off PR builds, and I will instead check the box for the repo settings that require branches to be up-to-date with the target in PRs. This has worked well for me in professional settings for repos that don't see multiple PR merges per day.

@@ -0,0 +1,45 @@
name: InstrumentKit CI

on: [push]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably extend this to also run on pull-requests. In addition, could specify to only run on master, i.e., on a push to master or a PR request to master, something like this:

on:  
  push:
  pull_request:
    branches:
      - master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mostly just been testing so far to see what works and doesn't


jobs:
test:
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

I had some issues with another project with github actions, namely that it also runs on forks. Now the {{ secrets.github_token }} won't be in that fork, and so the test will fail every time. You can limit that to only run if you are the owner, something like:

jobs:
  test:
    if: github.repository_owner == 'Galvant'
    runs-on: ubuntu-latest

Solution is taken from this discussion. Not sure if it's the most elegant one, but this way actions don't run and fail due to missing secrets on forks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting. That's a good idea for the coveralls stage.

# run: coveralls

- name: Coveralls
uses: AndreMiras/coveralls-python-action@develop
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was getting a lot of issues with the official coveralls action integration here due to the structure of the report that pytest --cov= generates. Apparently the solution is to use the coverage command directly, but I want things to be symmetric with tox. The author here solved this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely a neat solution

@coveralls
Copy link

coveralls commented Dec 19, 2020

Coverage Status

Coverage increased (+0.04%) to 99.404% when pulling f0c5c39 on migrate-to-github-actions into 4c3433a on master.

@scasagrande scasagrande force-pushed the migrate-to-github-actions branch 7 times, most recently from cfdd864 to c7aa720 Compare January 20, 2022 02:28
@scasagrande scasagrande marked this pull request as ready for review January 20, 2022 02:43
@scasagrande
Copy link
Contributor Author

The one failing status check should just be a stale one from testing. Clicking the "Details" shows the correct information.

name: Testing

on:
push:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this include testing when a new pull request is started? I saw that you had pull requests and push in there in 9f1a61d, but now only push to any branch, so I'm wondering if the full test suite including coverage will run on a new pull request that is pushed from a fork.

I have in another project limited push testing to the main branch and added a test requirement for pull requests to main as:

on:
  push:
    branches: [ main ]
  pull_request:
    branches: [ main ]

@trappitsch
Copy link
Contributor

Looks great, for fun I pushed this branch to my fork, github actions ran cleanly through, coverage didn't run at all.

@scasagrande
Copy link
Contributor Author

@trappitsch perfect I was just about to ask you to do that!

I do want the coverage to function, so lets try to figure that out.

@scasagrande
Copy link
Contributor Author

@trappitsch this should be your coveralls report from that build: https://coveralls.io/builds/45792067

@scasagrande
Copy link
Contributor Author

@trappitsch can you open a fake PR from your fork targetting this branch, and we can see how the status checks look

@trappitsch
Copy link
Contributor

Ah I see, the coverage is done in the task "finish". Here is this branch in my fork, you can see all actions pass:
https://github.com/trappitsch/InstrumentKit/tree/migrate-to-github-actions

I'll prep a fake PR from my fork to this branch, let's see what happens :)

@trappitsch
Copy link
Contributor

Fake PR is #315

@trappitsch
Copy link
Contributor

trappitsch commented Jan 20, 2022

This might be related for the tests running twice:
https://github.community/t/how-to-trigger-an-action-on-push-or-pull-request-but-not-both/16662/3

There seem to be some solutions...

@scasagrande
Copy link
Contributor Author

I've got a lot to learn here but I think this is better now. It should run on all PRs targetting any branch, and then run post-merge on the default branch.

@trappitsch
Copy link
Contributor

The commit message is indeed long and weird, but seems only for that one...
Otherwise this looks great!

@scasagrande
Copy link
Contributor Author

🚢

@scasagrande scasagrande merged commit f4021f8 into master Jan 20, 2022
@scasagrande scasagrande deleted the migrate-to-github-actions branch January 20, 2022 22:04
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.

3 participants