Support for metadata.generateName (CSA) #2594

EronWright · 2023-10-03T22:31:28Z

Proposed changes

This PR implements support for .metadata.generateName in CSA mode, based on #2790.

Adjust the auto-naming logic to consider .metadata.generateName to be a variant of auto-naming. Pulumi will not assign an auto-name to a new resource that has generateName, and upon delete will use the replace-then-delete technique.
Fix an autonaming bug where the old auto-name would erroneously be adopted, overwriting a computed name (ref). It is in-scope because there's significant interplay between autonames and generateName. (see Fix auto-naming bug for computed names #1618)

Tests

A new integration test is provided to test the use of metadata.generateName. It tests creation, update, replacement, and promotion from .generateName to .name. (ref)

The existing autonaming test is enhanced to test how autonaming takes precedence over generateName, in the update case. This is to ensure backwards compatibility, e.g. in the edge case that an existing object has a generateName field (which Pulumi ignored until now). (ref)

Related issues (optional)

github-actions · 2023-10-03T22:37:29Z

Does the PR have any schema changes?

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

codecov-commenter · 2023-10-03T22:39:23Z

Codecov Report

Attention: 161 lines in your changes are missing coverage. Please review.

Comparison is base (3c6513e) 23.41% compared to head (23bda1e) 23.30%.

Files	Patch %	Lines
provider/pkg/provider/provider.go	0.00%	53 Missing ⚠️
provider/pkg/await/awaiters.go	0.00%	30 Missing ⚠️
provider/pkg/await/await.go	0.00%	18 Missing ⚠️
provider/pkg/metadata/naming.go	33.33%	17 Missing and 1 partial ⚠️
provider/pkg/await/deployment.go	43.75%	9 Missing ⚠️
provider/pkg/await/ingress.go	11.11%	8 Missing ⚠️
provider/pkg/openapi/openapi.go	0.00%	7 Missing ⚠️
provider/pkg/await/job.go	0.00%	6 Missing ⚠️
provider/pkg/await/statefulset.go	14.28%	6 Missing ⚠️
provider/pkg/await/pod.go	0.00%	5 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2594      +/-   ##
==========================================
- Coverage   23.41%   23.30%   -0.12%     
==========================================
  Files          48       48              
  Lines        9629     9679      +50     
==========================================
+ Hits         2255     2256       +1     
- Misses       7224     7272      +48     
- Partials      150      151       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EronWright · 2023-10-05T23:17:55Z

@lblackstone could you give me some feedback on this PR? Two aspects of note:

Do you agree that the "auto-import" thing that happens when you using SSA is actually a bug? In order to use generateName, one must use create rather than patch. Pulumi should throw when an object already exists, unless you're using the Patch resource variant.
Why do the awaiters use the inputs and shouldn't they use the outputs? The inputs don't have the generated name, obviously.

lblackstone · 2023-10-16T15:22:55Z

Do you agree that the "auto-import" thing that happens when you using SSA is actually a bug? In order to use generateName, one must use create rather than patch. Pulumi should throw when an object already exists, unless you're using the Patch resource variant.

The "upsert" behavior was intentional with the idea that it's like running kubectl apply. If the cluster state already matches the declaration (desired state), then it shouldn't error. I think this difference was a side effect of using Create vs Patch previously. If we need to switch back to using Create for other reasons, we should see if it's possible to maintain this behavior where it succeeds if the state already matches the declaration. Alternatively, we could change the semantics to only upsert on Patch resources, but that is a break from the way the SSA behavior was designed and tested, so this would need to be done thoughtfully.

Why do the awaiters use the inputs and shouldn't they use the outputs? The inputs don't have the generated name, obviously.

Can you give me a specific example? From what I recall, all of the awaiters start watching event streams, so the input was used to figure out the name of the resource to watch. Computed outputs aren't available during preview, so that would be the most likely behavior change. It's possible that using outputs is more correct, but this is another change to take with care, since it could affect existing semantics.

lblackstone

I think these changes generally look ok, but with the caveats about possible behavior changes I mentioned earlier. The await code has been mostly stable for 3+ years, and is a core part of many resource updates, so this is an area to tread carefully. Particular testing care is needed for preview differences with computed inputs, and for resources that change between static names and the auto-named varieties.

provider/pkg/await/await.go

provider/pkg/provider/provider.go

disallow generateName for SSA and for yaml mode. use resource name rather than object name in certain error messages.

allow implicit to explicit name specification.

provider/pkg/await/await.go

provider/pkg/provider/provider.go

lblackstone · 2024-01-22T17:24:45Z

tests/sdk/nodejs/generatename/step2/index.ts

@@ -0,0 +1,36 @@
+// Copyright 2016-2019, Pulumi Corporation.


nit: outdated copyright

provider/pkg/await/await.go

rquitales · 2024-01-22T19:36:44Z

provider/pkg/await/awaiters.go

@@ -301,19 +301,19 @@ func untilAppsDeploymentDeleted(config deleteAwaitConfig) error {
 		specReplicas, _ := deploymentSpecReplicas(d)

 		return watcher.RetryableError(
-			fmt.Errorf("deployment %q still exists (%d / %d replicas exist)", config.currentInputs.GetName(),
+			fmt.Errorf("deployment %q still exists (%d / %d replicas exist)", d.GetName(),


Should we introduce a new method for our config structs that returns the name of the deployment object? This refactor would make it less error-prone when deciding on the deployment name to use, and will make it easier in the future should we decide on a different strategy for the deployment name (say, reverting back to using inputs).

rquitales · 2024-01-22T19:39:38Z

provider/pkg/metadata/naming.go

 	if newObj.GetName() == "" && IsAutonamed(oldObj) {
+		contract.Assertf(oldObj.GetName() != "", "expected nonempty name for object: %s", oldObj)


Incorrect formatting here? The unstructured object won't be nicely formatted into a string. Should the resource URN be printed here instead?

provider/pkg/provider/provider.go

EronWright · 2024-01-29T19:12:32Z

Closing this PR in favor of:

EronWright marked this pull request as draft October 3, 2023 22:40

EronWright requested a review from lblackstone October 5, 2023 23:14

lblackstone reviewed Oct 16, 2023

View reviewed changes

provider/pkg/await/await.go Outdated Show resolved Hide resolved

provider/pkg/provider/provider.go Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

EronWright added 4 commits January 16, 2024 12:32

Use output properties for await

9b648fa

replace on change to generateName.

5b51506

disallow generateName for SSA and for yaml mode. use resource name rather than object name in certain error messages.

assertions

497f0bb

use apply for SSA

db4891f

EronWright force-pushed the eronwright/issue-2539 branch from 02f8c0b to db4891f Compare January 18, 2024 18:55

EronWright added 7 commits January 18, 2024 11:47

patch to use live object name for logging

e6bb63e

do not replace on generateName change.

39ea869

allow implicit to explicit name specification.

special case for helm release

c4fd19d

bugfix for awaiters

a68eceb

Integration tests for generateName

870717b

improved back compat, support for computed inputs.

72f74c1

Merge remote-tracking branch 'origin/master' into eronwright/issue-2539

2ddeb33

EronWright requested a review from rquitales January 19, 2024 21:01

EronWright marked this pull request as ready for review January 19, 2024 21:01

EronWright changed the title ~~[WIP] Support for metadata.generateName~~ Support for metadata.generateName (CSA) Jan 19, 2024

changelog

ac08bdc

lblackstone reviewed Jan 22, 2024

View reviewed changes

rquitales requested changes Jan 22, 2024

View reviewed changes

mjeffryes assigned EronWright Jan 26, 2024

mjeffryes added this to the 0.99 milestone Jan 26, 2024

Merge remote-tracking branch 'origin/master' into eronwright/issue-2539

23bda1e

EronWright closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for metadata.generateName (CSA) #2594

Support for metadata.generateName (CSA) #2594

EronWright commented Oct 3, 2023 •

edited

github-actions bot commented Oct 3, 2023

codecov-commenter commented Oct 3, 2023 •

edited by codecov bot

EronWright commented Oct 5, 2023 •

edited

lblackstone commented Oct 16, 2023

lblackstone left a comment

This comment was marked as outdated.

lblackstone Jan 22, 2024

rquitales Jan 22, 2024

rquitales Jan 22, 2024

EronWright commented Jan 29, 2024 •

edited

		if newObj.GetName() == "" && IsAutonamed(oldObj) {
		contract.Assertf(oldObj.GetName() != "", "expected nonempty name for object: %s", oldObj)

Support for metadata.generateName (CSA) #2594

Support for metadata.generateName (CSA) #2594

Conversation

EronWright commented Oct 3, 2023 • edited

Proposed changes

Tests

Related issues (optional)

github-actions bot commented Oct 3, 2023

Does the PR have any schema changes?

codecov-commenter commented Oct 3, 2023 • edited by codecov bot

Codecov Report

EronWright commented Oct 5, 2023 • edited

lblackstone commented Oct 16, 2023

lblackstone left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

lblackstone Jan 22, 2024

Choose a reason for hiding this comment

rquitales Jan 22, 2024

Choose a reason for hiding this comment

rquitales Jan 22, 2024

Choose a reason for hiding this comment

EronWright commented Jan 29, 2024 • edited

EronWright commented Oct 3, 2023 •

edited

codecov-commenter commented Oct 3, 2023 •

edited by codecov bot

EronWright commented Oct 5, 2023 •

edited

EronWright commented Jan 29, 2024 •

edited