Run release from CI workflow

[victorlin]

Description of proposed changes

This leverages the existing test matrix setup for CI which can sufficiently replace devel/test. A downside is that it does not test the changes created by devel/release, but those should not have an impact on test results.

Related issue(s)

Closes #1633

Checklist

• Automated checks pass
• release job is skipped for PR run
• Check if you need to add a changelog message
• Check if you need to add tests
• Check if you need to update docs
• Post-merge: create issue for further improvements to release process (ref): CI/release: Run tests on build artifacts #1638
• Post-merge: next release happens successfully

[tsibley]

In general this seems fine. It's an improvement over the current thing in terms of testing. Note it does mean that the release process will take much longer (not a nonstarter, but noting it).

A downside is that it does not test the changes created by devel/release

It's not just the source code changes created by devel/release that aren't tested, but that the actual release distributions (also currently created by devel/release) aren't ever tested themselves. Only the source code form is tested.

In the future, I'd encourage reorganizing to a stricter CI/CD flow where distribution artifacts are always built and those artifacts are the things tested and ultimately released (if appropriate). (For an example, see what I did for Nextstrain CLI.)

[tsibley]

non-blocking

This is an oddly tight binding of the two workflows to me. If we're going to do it, let's condition on the workflow file path instead of its name (which seems much more subject to change, and thus breakage)?

Alternatively, why condition here at all beyond the workflow_call event?

[victorlin]

Updated to condition on file path + branch:

augur/.github/workflows/ci.yaml

Lines 359 to 360 in fc40285

	# Only run when called by the release workflow on the default branch
	if: github.event_name == 'workflow_call' && github.workflow_ref == format('{0}/.github/workflows/release.yaml@refs/heads/{1}', github.repository, github.event.repository.default_branch)

Alternatively, why condition here at all beyond the workflow_call event?

Conditioning only on workflow_call would mean "run release steps when the CI workflow is called by another workflow" which seems too loosely coupled. The CI workflow could be called by another workflow for other reasons in which release shouldn't happen.

If anything, I think the workflow_call condition is redundant and can be removed. The path being anything other than .github/workflows/ci.yaml implies workflow_call as the triggering event.

[tsibley]

Conditioning only on workflow_call would mean "run release steps when the CI workflow is called by another workflow" which seems too loosely coupled. The CI workflow could be called by another workflow for other reasons in which release shouldn't happen.

The workflow_call trigger requires a version input, so any call of it with a version seems to me like a release is intended.

In any case, the larger point is that I think the coupling here between the two workflows is weird and an artifact of trying to preserve the "Release" workflow.

[victorlin]

I see. I meant for this PR to be a small change to prevent release on failing tests. Maybe it'd be better to drop the "Release" workflow, but that needs a larger discussion around aligning release procedures between Augur/Auspice/Nextstrain CLI. I'll keep it in this PR.

The workflow_call trigger requires a version input, so any call of it with a version seems to me like a release is intended.

True, but when browsing the code I think (2) reads better than (1):

1. release:
     # Only run when the triggered by workflow_call with input.version
     if: github.event_name == 'workflow_call'
     needs: [pytest-cram]

2. release:
     # Only run when called by the release workflow on the default branch
     if: github.workflow_ref == format('{0}/.github/workflows/release.yaml@refs/heads/{1}', github.repository, github.event.repository.default_branch)
     needs: [pytest-cram]

Checking default branch is important. With (1) it could happen in release.yaml, but (2) comes with added context of actual release steps.

[tsibley]

I meant for this PR to be a small change to prevent release on failing tests.

Yep, and that's fine, hence why I said at the top: "In general this seems fine. It's an improvement over the current thing in terms of testing." :-)

[victorlin]

the actual release distributions (also currently created by devel/release) aren't ever tested themselves

That is already the case prior to this PR, but point taken. I'll create a separate issue for improvements once this PR is merged.