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

Verify release action #977

Merged
merged 7 commits into from
Jun 21, 2024
Merged

Verify release action #977

merged 7 commits into from
Jun 21, 2024

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Jun 18, 2024

Add new job in our release workflows to verify that our released SDKs and provider are usable.

This will have to be configured into each provider as we will need a program specified in the local repo for each runtime we want to verify. Ideally this will include: nodejs, python, dotnet and Go. Without configuration, the job will not do any verification. There's an example of the available configuration as documentation in the bridged-provider.config.yaml.

The verify-release workflow is also runnable directly if we want to do a one-off verification. See the pulumi/verify-provider-release action for the underlying implementation.

Fixes #972

Example run without configuration skips the job:

image

Example prerelease with nodejs configuration runs on Ubuntu and Windows:

image

@danielrbradley danielrbradley self-assigned this Jun 18, 2024
@danielrbradley danielrbradley changed the base branch from master to single-version-calc June 18, 2024 20:11
@AaronFriel
Copy link
Contributor

About how many CI minutes of macOS runner time would be incurred weekly from running verify release?

I imagine we can estimate # of releases weekly * length of verification.

I ask because we have a maximum concurrency of 5 macOS runners on OSS repositories and this could result in CI brownouts for pulumi/pulumi.

Cc @mjeffryes

@mjeffryes
Copy link
Member

This looks like it adds verification after prerelease, so we'd be looking at num_provider_commits * verification_time. We closed 866 PRs in providers last week, most probably were merged and ran prerelease. Even if verification time is < 2min, it's going to burn a fair amount. We could potentially only test windows and mac on a full release to relive some pressure there.

@danielrbradley
Copy link
Member Author

I'll disable verifying pre-releases on Mac for the time being - only verify them on Mac after stable releases. This should suffice but still provide fast feedback on any issues.

Base automatically changed from single-version-calc to master June 20, 2024 08:46
Make release verification opt-in as it needs configuration for the programs to run.

Add example configuration as documentation.
@danielrbradley danielrbradley requested a review from a team June 20, 2024 09:10
- Rename tag_sdk to publish_go_sdk to match pre-release (and what we're actually doing).
- Add explicit enableMacosRunner input for prerelease for explicit documentation of behaviour.
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

What's the rollout plan here? @danielrbradley will you roll this out to all providers? To tier 1 providers?

Do we intend to roll this out to all providers eventually? If so, we will need to budget for that (not all providers have tests at all right now). If not, then we probably shouldn't warn when a test is missing:

    - run: echo "::warning file=.ci-mgmt.yaml,title=Python release verification missing::Add the key releaseVerification.python to .ci-mgmt.yaml pointing to the directory containing a Pulumi python project to preview."

verify-release:
name: verify-release
strategy:
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

It feels like we could expand this matrix to also include language. That would give us parallelism and reduce repeated code.

You might need to build a simple lookup table to map #{{ .Config.releaseVerification.nodejs }}# to nodejs, perhaps using GH action outputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had considered putting the runtime in the matrix, but given we're only doing a preview, this is a very fast run - most of the time is spent just installing dependencies. We could tweak the way it runs so we still report all the tests even if earlier steps fail if we like.

@danielrbradley
Copy link
Member Author

danielrbradley commented Jun 20, 2024

What's the rollout plan here? @danielrbradley will you roll this out to all providers? To tier 1 providers?

Initially, this is just rolling out the machinery to be able to start implementing this in each provider. Tier 1 are obviously the priority, but this should really be implemented for all providers given we're not actually deploying any real cloud resources - only validating the SDK and provider setup.

If the warnings are too messy then we could tidy these up a bit into a single warning perhaps, but it would make for more messy ci-mgmt code. These warnings are also only visible in the actual release workflow run view, so they really don't actually get in the way too much.

@iwahbe
Copy link
Member

iwahbe commented Jun 20, 2024

I'd prefer to not introduce a class of warnings unless we plan to address them (immediately), since it builds the habit of ignoring warnings. That said, we already ignore plenty of build warnings, so it's probably not that much worse here. We can always disable them later if we need to.

Remove printing of warnings and instead just mark the whole job as skipped if not configured. Move the setup instructions into a workflow comment.
@danielrbradley
Copy link
Member Author

@iwahbe I've updated the code to completely skip the verify-release job if it's not implemented instead of the warnings

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.

Post-release test
5 participants