chore(ci): Fix python release cycle

[wackywendell]

This fixes the build for Python releases, which will hopefully resolve #296.

We may need to change the tag for v0.1.0 for this to work; alternatively, we could release v0.1.1 if we can't change the tag.

[mbrobbel]

I think you should add both github.ref (to capture the difference between branch and tag push events) and strategy.job-index (which is different for every job in the matrix) to make this work.

Suggested change

	name: wheel-${{ matrix.os }}-${{ matrix.python-version }}
	name: wheel-${{ github.ref }}-${{ strategy.job-index }}

Also note that now you are only running the uploads once (the pull request trigger), and you're seeing the failure because the sdist job has the same matrix.os and matrix.python-version pair as the wheel job for that combo.

[wackywendell]

Whoa, you're way ahead of me here, thanks! Will do.

[wackywendell]

Hm, I think there are slashes in github.ref, which aren't allowed.

The suggestion here is to explicitly use the matrix components, so I might do that. I'll push some more on this tomorrow.

[wackywendell]

Blargh, that was harder than I expected to get CI to work. I'm going to clean up, rebase and squash a bit, and then mark this ready for review.

[mbrobbel]

I think we should be more specific here to prevent downloading artifacts from the non-tag workflow run. I'm unsure what would happen to the duplicates if we do it like this.

[wackywendell]

My understanding of Github Artifacts is that artifacts are shared within a workflow, but not across workflows - so that while the branch workflow and tag workflow might have artifacts with the same name, they would not be able to see each other.

From the docs:

Artifacts allow you to persist data after a job has completed, and share that data with another job in the same workflow.

See also the download-artifact docs, which discuss adding a tokens and run id to the download-artifact action to enable downloading from other runs (which we don't want to do).

[mbrobbel]

I think we should at least include something that is different for tag and branch jobs.

[wackywendell]

See above - as I understand it, tag and branch jobs would be in different workflows, and would not have access to each others' artifacts without specifically specifying a run-id and adding a GITHUB_TOKEN to the action. Does that make sense? Am I missing something?

[wackywendell]

@mbrobbel - thanks for the quick reviews! It looks like we're understanding how Github namespaces artifacts, jobs, and workflows differently, so let's try and resolve that - see comments above.

Additionally - from my own perspective, adding the "dry run" features is useful for this PR to see if it 'works', but means adding a bunch of unnecessary artifacts to every pull request, and also the above note "@wackywendell deployed to VALIDATOR_RELEASE — with GitHub Actions" doesn't seem right. So I'm dropping the dry run "feature", so that going forward PRs will continue to not upload, download, or attempt to 'release' artifacts.

Does that make sense? What are your thoughts?

[mbrobbel]

@mbrobbel - thanks for the quick reviews! It looks like we're understanding how Github namespaces artifacts, jobs, and workflows differently, so let's try and resolve that - see comments above.

Additionally - from my own perspective, adding the "dry run" features is useful for this PR to see if it 'works', but means adding a bunch of unnecessary artifacts to every pull request, and also the above note "@wackywendell deployed to VALIDATOR_RELEASE — with GitHub Actions" doesn't seem right. So I'm dropping the dry run "feature", so that going forward PRs will continue to not upload, download, or attempt to 'release' artifacts.

Does that make sense? What are your thoughts?

Ah, I was under the assumption that we also upload artifacts for non-tag workflows, but I missed

substrait-validator/.github/workflows/python.yml

Line 99 in 0ffbb83

if: "startsWith(github.ref, 'refs/tags/')"

.

Now it makes sense, it was just failing because we bumped but never tested after that, and it had nothing to do with the other job that ran for the same commit.

Anyway, I'm happy to merge this PR and release 0.1.1. What are you thoughts? Do you want to prepare the 0.1.1 PR?

[mbrobbel]

Thanks for fixing this!

[wackywendell]

Sure - I'll make a 0.1.1 release PR, if you can merge this and then release that. Thanks!