Introduce provider upgrade tests #2714

t0yv0 · 2023-08-25T13:38:47Z

Partially fixes #2719

This PR adds coverage for testing provider upgrades from a baseline version (currently 5.42.0) to the in-source release candidate. The tests check that there are no upgrade or replace plans when upgrading the provider. This is verified on a few select resources.

Currently tested resources include:

aws:ec2/subnet:Subnet
aws:ec2/vpc:Vpc
aws:ec2:SecurityGroup
aws:eks:Cluster
aws:iam:Role
aws:iam:RolePolicyAttachment
aws:rds/instance:Instance
aws:route53:ResolverEndpoint
aws:s3/bucket:Bucket

The test relies on recording and replaying the behavior of the baseline version. There are two versions of the test, in-memory TestProviderUpgradeQuick version and full acceptance-style TestProviderUpgrade version. Full version is skipped for the moment with references to discovered issues.

github-actions · 2023-08-25T14:04:53Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

t0yv0 · 2023-08-25T14:41:34Z

@iwahbe I've pondered sequencing here and I think if there's ongoing work to bring AWS repo under ci-mgmt perhaps this PR is not a good time to split examples and tests.

I don't think we were tracking this before so here's a new issue:

pulumi/ci-mgmt#526

Once pulumi-aws is under ci-mgmt we can roll out examples/tests split everywhere including here, then move tests around.

Makefile

t0yv0 · 2023-08-25T14:43:43Z

Let me confirm but I also think this is self-sufficient for recording v5 provider; the baseline version is currently written in the Pulumi.yaml Program file. So if doing something like adding example resources we should be able to re-record this baseline version from the main branch without having to checkout some earlier branch.

examples/provider_upgrade_test.go

iwahbe · 2023-08-25T16:42:03Z

examples/provider_upgrade_test.go

+		}
+
+		if isPureMethod(t, line) {
+			line = ignoreStables(t, line)


This is fine for now, but we should remember to make stables stable and then remove this, since this field is effectively no longer under test.

iwahbe · 2023-08-25T16:45:24Z

examples/provider_upgrade_test.go

+	ambientProvider, _ := exec.LookPath(providerBinary)
+	require.Emptyf(t, ambientProvider, "please remove the provider from PATH")


Instead of this, can we just set PULUMI_IGNORE_AMBIENT_PLUGINS=true? It should accomplish the same thing without requiring destructive user action.

Hmm. Not sure, I prefer this I think to fail loudly. Perhaps if we went PULUMI_IGNORE_AMBIENT_PLUGINS way I'd like another check to fail loudly if baseline recording is not on the expected version.

examples/provider_upgrade_test.go

iwahbe · 2023-08-25T17:02:44Z

examples/provider_upgrade_test.go

+	info := providerUpgradeInfo{}
+	info.testCaseDir = filepath.Join("testdata", "resources")
+	pyaml := filepath.Join(info.testCaseDir, "Pulumi.yaml")
+	v := parseProviderVersion(t, pyaml)


We will eventually want to provision multiple providers. The obvious example is lots of tags configurations. We can leave it for now and revisit.

I'd leave it, I'm not sure how to avoid copy-pasting resource configurations, perhaps we can parameterize the stacks somehow or resort to YAML munging. We might also need to break down this single stack into N stacks. I'd like to check in a reasonably working first version before making those extensions though.

iwahbe · 2023-08-25T17:11:03Z

examples/provider_upgrade_test.go

+	pt.RunPulumiCommand("stack", "export", "--file", newStateFile)
+	stackName := parseStackName(t, readFile(t, newStateFile))
+	fixedState := withUpdatedStackName(t, stackName, readFile(t, stateFile))


We read the new state file twice, and write it once. Can we do this?

Suggested change

pt.RunPulumiCommand("stack", "export", "--file", newStateFile)

stackName := parseStackName(t, readFile(t, newStateFile))

fixedState := withUpdatedStackName(t, stackName, readFile(t, stateFile))

pt.RunPulumiCommand("stack", "export", "--file", newStateFile)

newStateFileBytes := readFile(t, newStateFile)

stackName := parseStackName(t, newStateFileBytes)

fixedState := withUpdatedStackName(t, stackName, newStateFileBytes)

Well no, I think in fact this would invalidate the test.. We want to read the pre-recorded stateFile (from stateFile variable). We're only reading the newStateFile as a workaround to determine currently allocated stack's name. I can factor it out into a function to make more obvious.

examples/provider_upgrade_test.go

mjeffryes

I have many comments and questions, but nothing blocking. This seems like a useful tool, and something where we can safely land it and continue to improve

mjeffryes · 2023-08-28T18:07:07Z

Makefile

+
+# Runs integration tests on the baseline version and updates testdata to record expected behavior.
+test.upgrade.record::
+	cd examples && PULUMI_ACCEPT=true go test -v -tags all -run TestProviderUpgradeRecord -timeout 2h


The Makefile feels like a non-ideal place to define a 2hr timeout. I would expect to define a timeout in the workflow for a CI job or just rely on the user for a timeout when running locally?

This is the state of the art in the Makefile already :) Just following existing practice.

mjeffryes · 2023-08-28T18:18:14Z

examples/provider_upgrade_test.go

+)
+
+const (
+	acceptEnvVar   = "PULUMI_ACCEPT"


The name of this variable is pretty generic for something that just toggles between record and compare modes.

The name is consistent with similar tests in pulumi/pulumi and pulumi/pulumi-{yaml, java}.

https://github.com/hexops/autogold defines custom -update flag which is an interesting approach we could use too. I'm just indeed opting for consistency with pulumi/pulumi where we use this strange PULUMI_ACCEPT var.

mjeffryes · 2023-08-28T18:30:05Z

examples/provider_upgrade_test.go

+func TestProviderUpgradeRecord(t *testing.T) {
+	if !accept {
+		t.Skipf("Skipping; to record baselines set %s env var to true", acceptEnvVar)
+	}


I wonder if this should just be a separate binary rather than a test? I can see how writing it as a test makes it easier to get a testing.T for integration.ProgramTest(t, &test) below, but it also means you have to have the extra env var to keep it from running most of the time. If it's not a huge amount of work to just make it run as a separate command, that could be a bit easier to maintain in the long term.

We normally have this in a single test, that is always run in either a "record" or "replay" mode. We might want to do that here, since TestProviderUpgradeQuick and TestProviderUpgradeRecord interact via the file system.

Since it never makes sense to run both tests in 1 go, we might want to merge them here as well.

Edited for it to sit in a single test. I do think refreshing belongs in the test world and not a separate binary, more idiomatic Go it seems, see https://github.com/hexops/autogold again.

examples/provider_upgrade_test.go

mjeffryes · 2023-08-28T20:44:00Z

examples/provider_upgrade_test.go

+
+func providerServer(t *testing.T) pulumirpc.ResourceProviderServer {
+	ctx := context.Background()
+	version.Version = "0.0.1"


probably just a go n00b type question, but I don't see where version is defined, or understand why you're setting it here?

version.Version is a global variable, normally set by the linker. We set it here because we expect that version.Version is valid semver (which "" is not).

iwahbe

I think we can merge as is, and this will provider a useful backstop.

In the future, I think we should talk about the test structure. In particular, I don't understand why we have 3 test functions (with separate code) when I can only remember us wanting two tests.

Before we merge this PR, I would like all 3 tests to have a doc comment explaining what they are doing and how to run them (PULUMI_ACCEPT, etc.).

iwahbe · 2023-08-28T21:36:11Z

examples/provider_upgrade_test.go

+func TestProviderUpgradeRecord(t *testing.T) {
+	if !accept {
+		t.Skipf("Skipping; to record baselines set %s env var to true", acceptEnvVar)
+	}


We normally have this in a single test, that is always run in either a "record" or "replay" mode. We might want to do that here, since TestProviderUpgradeQuick and TestProviderUpgradeRecord interact via the file system.

Since it never makes sense to run both tests in 1 go, we might want to merge them here as well.

examples/provider_upgrade_test.go

t0yv0 added 15 commits August 23, 2023 15:58

Initial work

bce00ed

Fix stack name discrepancies

861f00f

Remove redundant

a4554cd

Sanity checks on ambient vs released provider

cd22d45

Log baseline version

d694f77

Add friendly Make targets

10c1b52

Fix quick test

0e47acd

Implement -short unit testing

9e0cc7a

Column width

0954a08

Note limitations

75f497e

Make test names consistent

132d28f

Record AWS instance data

ad7045e

Merge branch 't0yv0/upgrade-tests' into t0yv0/upgrade-tests-v6

9a1ff28

go mod tidy

ada6650

Upgrade to V6

7b39f9f

t0yv0 requested a review from iwahbe August 25, 2023 13:38

t0yv0 added 3 commits August 25, 2023 10:28

Move under examples/ so we do not need to edit CI

9173eee

Fix Go module name

5c7661b

Headers and docs

47d921d

t0yv0 force-pushed the t0yv0/upgrade-tests-v6 branch from f5e308b to 47d921d Compare August 25, 2023 14:34

t0yv0 added 3 commits August 25, 2023 10:35

Typo

b0328a9

Fix makefile target

d0db4ca

Drop ./tests for now

4d58b80

t0yv0 commented Aug 25, 2023

View reviewed changes

Makefile Show resolved Hide resolved

Run as part of java test set instead of nodejs test set

34f9bcf

iwahbe reviewed Aug 25, 2023

View reviewed changes

Fix record target

f5b7176

t0yv0 added 5 commits August 25, 2023 15:44

PR feedback: notes around isDiffRecord

3bd18a3

Check for empty URN

80bac60

Elaborate code based on PR feedback

4cacd60

Lint: unneeded return

bd4758b

PR nits

16c4c80

t0yv0 mentioned this pull request Aug 25, 2023

Provider Upgrade Testing #2719

Closed

12 tasks

t0yv0 added 9 commits August 25, 2023 17:14

Typo

b46d88e

Add tests for route53 resolverendpoint

92706f2

Enable quick replay of upgrade tests

d85e8e6

Add a comment

ebe4463

Merged eks.Cluster tests

a5646cd

Merge route53ResolverEndpoint tests

54c01ca

Fix record target

e477bb3

Re-record resource upgrade baselines

7b9d238

Note why we are skipping tests

f7d2295

t0yv0 changed the title ~~Introduce provider upgrade tests for the V6 RC branch~~ Introduce provider upgrade tests Aug 28, 2023

t0yv0 requested review from iwahbe, a team and mikhailshilkov August 28, 2023 16:48

t0yv0 marked this pull request as ready for review August 28, 2023 16:50

mjeffryes approved these changes Aug 28, 2023

View reviewed changes

iwahbe approved these changes Aug 28, 2023

View reviewed changes

t0yv0 added 5 commits August 31, 2023 15:55

Merged master

dc9a146

Rewrite preview-only tests for clarity

f20f3ef

Consolidate in one test

65d3cdc

Docs/makefile updates

7792abe

Make pre-recording faster

1107e67

t0yv0 merged commit 8e8d685 into master Sep 1, 2023
15 checks passed

t0yv0 deleted the t0yv0/upgrade-tests-v6 branch September 1, 2023 00:56

guineveresaenger mentioned this pull request Sep 5, 2023

Remove upstream fork and normalize Makefile #2784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce provider upgrade tests #2714

Introduce provider upgrade tests #2714

t0yv0 commented Aug 25, 2023 •

edited

Loading

github-actions bot commented Aug 25, 2023 •

edited

Loading

t0yv0 commented Aug 25, 2023

t0yv0 commented Aug 25, 2023

iwahbe Aug 25, 2023

iwahbe Aug 25, 2023

t0yv0 Aug 25, 2023

iwahbe Aug 25, 2023

t0yv0 Aug 25, 2023

iwahbe Aug 25, 2023

t0yv0 Aug 25, 2023

mjeffryes left a comment

mjeffryes Aug 28, 2023

t0yv0 Aug 31, 2023

mjeffryes Aug 28, 2023

iwahbe Aug 28, 2023

t0yv0 Aug 31, 2023

mjeffryes Aug 28, 2023

iwahbe Aug 28, 2023

t0yv0 Aug 31, 2023

mjeffryes Aug 28, 2023

iwahbe Aug 28, 2023

iwahbe left a comment

iwahbe Aug 28, 2023

		ambientProvider, _ := exec.LookPath(providerBinary)
		require.Emptyf(t, ambientProvider, "please remove the provider from PATH")

Introduce provider upgrade tests #2714

Introduce provider upgrade tests #2714

Conversation

t0yv0 commented Aug 25, 2023 • edited Loading

github-actions bot commented Aug 25, 2023 • edited Loading

Does the PR have any schema changes?

Does the PR have any schema changes?

t0yv0 commented Aug 25, 2023

t0yv0 commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjeffryes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 25, 2023 •

edited

Loading

github-actions bot commented Aug 25, 2023 •

edited

Loading