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

Add workflow for downstream tests #192

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

CiaranOMara
Copy link
Member

This PR adds a GitHub workflow to test active direct dependents of BioSequences. The workflow is modelled on https://github.com/FluxML/Zygote.jl/blob/master/.github/workflows/Downstream.yml.
The value for the group parameter in the package matrix is set as BioSequences. See FluxML/Flux.jl#1555 (comment) for an example of the group parameter in use. Setting the parameter as BioSequences will give direct dependents a free opportunity to provide a reduced test set without the need to come back and change anything here.

Example output: https://github.com/CiaranOMara/BioSequences.jl/actions/runs/1358606131

@CiaranOMara
Copy link
Member Author

@benjward and @jakobnissen, are there any issues with the included repositories?

Can this PR be inserted into the tree directly after tag v2.0.5?

The sequence of operations would be

  • create a new branch at the current head of master called develop or something;
  • hard reset master to v2.0.5; and
  • then merge this.

If there is an agreement, I'm comfortable performing these operations.

@kescobo
Copy link
Member

kescobo commented Oct 20, 2021

Seems a bit troubling that the unit tests are failing, as are a bunch of downstream packages, but that's not the fault of this PR, and I think worth knowing. I'm in favor of merging

One question I have is whether it makes sense to run this on every single commit of a PR, or whether it would be desirable / possible to manually trigger. Seems like a lot of extra compute. One argument against this idea is that it reduces the chance that it will actually get run, but I could imaging a workflow where we require it to run before merging a PR or something.

on:
push:
branches: [master]
tags: [v*]
Copy link

@pdimens pdimens Oct 20, 2021

Choose a reason for hiding this comment

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

@kescobo doesn't this tag trigger the Action when a new version is published, not just any ol' PR/Push?

Copy link
Member

Choose a reason for hiding this comment

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

It's the next line (pull_request:) that will cause it to trigger on each change to a PR.

In other words. If you make 5 commits locally, then open the PR, it will run once. But every time you push one or more commits to that PR, it will trigger again.

You can actually see it running on this PR...

Copy link

Choose a reason for hiding this comment

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

oy. less than ideal

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we give it a shot on PRs, since it doesn't seem to use many minutes. If it gets annoying we can change it later?

@pdimens
Copy link

pdimens commented Oct 20, 2021

This is brilliant, btw. I'm definitely adding this to PopGenCore.jl

@jakobnissen
Copy link
Member

I think this is a good idea. I'm surprised it only takes 11 minutes of CI.
I think we have 2000 minutes for the org per month, so we probably won't run out.

@CiaranOMara
Copy link
Member Author

CiaranOMara commented Oct 31, 2021

We haven't had a response to the following.

Can this PR be inserted into the tree directly after tag v2.0.5?

The sequence of operations would be

create a new branch at the current head of master called develop or something;
hard reset master to v2.0.5; and
then merge this.
If there is an agreement, I'm comfortable performing these operations.

Further to this, I'm happy to rebase upstream work too.

The reason for performing this type of insertion is to have certainty around any features or hotfixes/patches released before the breaking v3 release as there are downstream packages that are currently failing. If there is no intention to release features or hotfixes/patches before v3, I don't see any reason to insert this PR directly after v2.0.5 and this PR can be merged to the current head.

@TransGirlCodes
Copy link
Member

I think its fine to insert this after 2.0.5 and then merge it into master/v3. It's a github workflow and so won't interfere with any other part of the project.

Thanks for doing this, it's a good idea. I'm nor sure about it running on every PR, but if we have it running on master and tags, (or perhaps even on a timer basis?) and we basically consult it before releasing any version to avoid whacking another project that depends on us?

@TransGirlCodes
Copy link
Member

Ok, there is a master-bak branch to save the current state of master, and master has been hard reset.

on:
push:
branches: [master]
tags: [v*]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we give it a shot on PRs, since it doesn't seem to use many minutes. If it gets annoying we can change it later?

@TransGirlCodes TransGirlCodes merged commit 6d65c17 into BioJulia:master Nov 1, 2021
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.

5 participants