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 assertPrerelease option to stop releases outside of release.yml #30

Merged
merged 8 commits into from
Jun 18, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Jun 10, 2024

We accidentally releases one of our providers
artifactory by merging into main (without running the release workflow). This left the provider in an invalid state, since main did not publish a release provider binary, but it did publish a release SDK.

The solution is to prevent publishing release SDKs in any workflow besides release.yml.

As a follow up, I will add assertPrerelease: true to all workflows except release.yml.

if: assertPrerelease
run: |
if [[ "${{ inputs.version }}" =~ "^v[0-9]+\.[0-9]+\.[0-9]+$" ]] then
echo "It looks like you have attempted to publish a non-alpha release by accident."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love there to be a link to this tool's README but am honestly not sure how to do it succinctly.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

I think this is a sensible addition, but it wouldn't have stopped the publishing of the non-prerelease nuget package. You can see this by looking at the usages of inputs.version - it's only passed down into the Java publishing workflow - it's not used by the dotnet, python or node publishing actions.

The version passed into the publish action for artifactory was a correct pre-release version, but that version isn't used for the dotnet nuget release - the version number is already in the .nupkg file. This was caused by the makefile overriding the PROVIDER_VERSION variable that was set by CI - and building an asset with the major version number in.

A complete fix here would also require passing version down to each language-specific action (ideally as inputs not env) and in each verify that the package they're about to publish matches the version passed in.

@iwahbe
Copy link
Member Author

iwahbe commented Jun 11, 2024

I think this is a sensible addition, but it wouldn't have stopped the publishing of the non-prerelease nuget package. You can see this by looking at the usages of inputs.version - it's only passed down into the Java publishing workflow - it's not used by the dotnet, python or node publishing actions.

The version passed into the publish action for artifactory was a correct pre-release version, but that version isn't used for the dotnet nuget release - the version number is already in the .nupkg file. This was caused by the makefile overriding the PROVIDER_VERSION variable that was set by CI - and building an asset with the major version number in.

A complete fix here would also require passing version down to each language-specific action (ideally as inputs not env) and in each verify that the package they're about to publish matches the version passed in.

Thank's for pointing that out! Good catch.

We accidentally releases one of our providers
[artifactory](https://github.com/pulumi/pulumi-artifactory/actions/runs/9451955578/job/26035510190)
by merging into `main` (without running the release workflow). This left the provider in
an invalid state, since `main` did not publish a release provider binary, but it did
publish a release SDK.

The solution is to prevent publishing release SDKs in any workflow besides `release.yml`.
@iwahbe iwahbe force-pushed the iwahbe/assert-prerelease branch from 6a3bd50 to 8638d9d Compare June 11, 2024 22:44
@iwahbe iwahbe requested a review from danielrbradley June 11, 2024 22:44
@iwahbe
Copy link
Member Author

iwahbe commented Jun 11, 2024

A complete fix here would also require passing version down to each language-specific action (ideally as inputs not env) and in each verify that the package they're about to publish matches the version passed in.

It's not trivial to do that, especially for python and dotnet. I have added validation to nodejs (which published first), asserting that the version to be published matches ${{ inputs.version }}. This doesn't ensure safety, but it makes it much more likely, since if all SDKs are built with the same version, this will catch the problem.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

Yup, this will suffice for now as it would have prevented the previous failure.

As a side-note, given this is an action rather than a re-usable workflow, perhaps we shouldn't be installing the tools ourselves, but assume the caller should have set up the tools themselves (as we do now using the shared workflow) to avoid fragmentation (e.g. configuring cache settings etc) and modifying the caller's environment. Alternatively, we could model this as a workflow so we're creating our own clean environment.

Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

lgtm

lang/nodejs/action.yml Outdated Show resolved Hide resolved
@iwahbe
Copy link
Member Author

iwahbe commented Jun 17, 2024

As a side-note, given this is an action rather than a re-usable workflow, perhaps we shouldn't be installing the tools ourselves, but assume the caller should have set up the tools themselves (as we do now using the shared workflow) to avoid fragmentation (e.g. configuring cache settings etc) and modifying the caller's environment. Alternatively, we could model this as a workflow so we're creating our own clean environment.

I think we should model this as a workflow for full self-containment, but that is a separate PR.

iwahbe added a commit to pulumi/pulumi-artifactory that referenced this pull request Jun 17, 2024
@iwahbe iwahbe merged commit dceb5ea into main Jun 18, 2024
1 check passed
@iwahbe iwahbe deleted the iwahbe/assert-prerelease branch June 18, 2024 23:54
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.

4 participants