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

Test the provider binaries we're planning to release #967

Closed
danielrbradley opened this issue Jun 11, 2024 · 2 comments · Fixed by #1196
Closed

Test the provider binaries we're planning to release #967

danielrbradley opened this issue Jun 11, 2024 · 2 comments · Fixed by #1196
Assignees
Labels
impact/reliability Something that feels unreliable or flaky kind/enhancement Improvements or new features p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed

Comments

@danielrbradley
Copy link
Member

Currently, we build one provider binary during prerequisites which is captured as an asset and used for running our integration tests against. However, we then discard this binary and use goreleaser to build brand new binaries at the point of release which are not tested.

We should build the provider binaries for all platforms at the beginning of the workflow, capture them as asstes, use them for integration tests, then publish the tested provider binaries.

This will either require the removal of goreleaser (see Azure Native for how to release without goreleaser) or pay for their premium product.

@danielrbradley danielrbradley added impact/reliability Something that feels unreliable or flaky kind/enhancement Improvements or new features labels Jun 13, 2024
danielrbradley added a commit that referenced this issue Jul 3, 2024
Stop using goreleaser, improve our release notes, speed up releases.

- Fixes #1013
- Closes #816
- Partially addresses #731
- Related to #967

This should also save 3 hours of waiting per release of AWS.

Build cross-platform provider binaries, in parallel, straight after
prerequisites, then only calculate hashes & push to S3 and GitHub
releases directly during the publish job.

Specifics:

- Use GitHub's own release notes generator which allows us to suppliment
with our own context - specifically the schema change since the previous
latest release.
- Capture and restore the `schema-embed.json` to avoid having to run
`tfgen` on every provider build.
- Build the multi-platform build straight into the makefile for easier
local testing.
- Remove clearing disk space during publish as we're not doing any build
work there any more.

This does not include reworking tests to use the the provider binary
being released - but this can be added later fairly easily.
@mjeffryes mjeffryes modified the milestone: 0.107 Jul 24, 2024
@t0yv0
Copy link
Member

t0yv0 commented Nov 26, 2024

Goreleaser is removed but we are still double-building and testing the wrong binaries. Could be a very good change.

@blampe
Copy link
Contributor

blampe commented Dec 2, 2024

Would have prevented this p0 pulumi/pulumi-aws#4863

Maybe a dupe of #1033?

t0yv0 added a commit to pulumi/pulumi-aws that referenced this issue Dec 2, 2024
In
ea37dae
an error was introduced invalidating the production provider build - a
subtle error by moving a `go:embed` directory to wildcards and FS type.

Per https://pkg.go.dev/embed#hdr-Strings_and_Bytes the path to a
go:embed directory is a pattern. Apparently in the case of pulumi-aws CI
the binary is built twice, once for testing and once for production
release. In the for-testing build environment, schema-embed.json is a
file which works as expected. In the for-production build environment,
schema-embed.json is a folder created by the download-artifacts action
that contains a single file. Before the change, this code worked
correctly in both cases:

```
//go:embed schema-embed.json
var pulumiSchema []byte
```

After the change, unfortunately the replacement code worked only for the
for-test build scenario where schema-embed.json is a straight file,
while failing in production.

This change goes back to using the `[]byte` version of go:embed and
makes sure the builds recompute the minimal schema as necessary for both
builds to succeed.

This fixes #4863

We still should address the root cause and refactor the builds to reduce
double-building the provider, so that the exact release build is
qualified by integration tests; which is tracked in
pulumi/ci-mgmt#967
@t0yv0 t0yv0 self-assigned this Dec 3, 2024
@joeduffy joeduffy added the p1 A bug severe enough to be the next item assigned to an engineer label Dec 3, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 3, 2024
Fixes #967 
Fixes #1033

Unlike the ticket spec in #967 , only linux-amd64 binary is tested by
the full set of integration tests. We might consider opening a follow up
to smoke-test every single one of the platform builds, or else keep #967
open.

As to the ticket spec in #1033 the dependency from build_provider to
prerequisites is real because prerequisites builds the schema and the
schema is needed in the binaries. This needs to stay.

Test run: pulumi/pulumi-azuread#1589

Additional exploration: in prerequisites we run `make test_provider`
which seems to try running unit tests in the `./provider` directory.
Unfortunately some tests in `./provider` directory are also integration
tests, e.g. in pulumi-aws and pulumi-azuread, and the convention of
passing `go test -short` to only run the unit tests does not seem to be
observed universally. For this reason removing `make provider` from
prerequisites is not yet possible as it breaks our builds. This is
likely out of scope of this PR.
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/reliability Something that feels unreliable or flaky kind/enhancement Improvements or new features p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants