Deprecate `name` on `aws.rds.Instance` #2686

iwahbe · 2023-08-09T10:56:36Z

We should have deprecated name in v5.x. We could then remove it here. Because we didn't remove it here, we need to deprecate it now, so we can remove at v7.0.0.

We want the following properties when we deprecate name:

Doesn't require a patch upstream.
name is deprecated in the schema.
name warns when used, directing the user to use dbName instead.
name is still valid as an input.
name is still valid as an output.

This PR works by back-porting name into the upstream schema from the Provider() tfbridge.ProviderInfo function. Since the upstream provider is no longer aware of name, we set dbName to the value of name and delete name. If both were provided, we error, just like v5.42.0 did.

As implemented, we get:

Doesn't require a patch upstream.
name is deprecated in the schema.
name warns when used, directing the user to use dbName instead.
name is still valid as an input.
name is still valid as an output.

To warn when we need to, we need to patch the bridge to allow passing a logger into PreCheckCallback. This change will be non-breaking.

github-actions · 2023-08-09T11:08:36Z

Does the PR have any schema changes?

Found 1 breaking change:

Resources

🟢 "aws:rds/instance:Instance": required: "name" property is no longer Required
No new resources/functions.

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

guineveresaenger · 2023-08-09T12:32:01Z

provider/resources.go

+				if err != nil {
+					return nil, err
+				}
+				return applyTags(ctx, config, meta)


Just to make sure I understand the logic here:

if a resource that hits this code path has a non-nil callback, we want to apply tags after applying the existing callback function to the config?

if a resource that hits this code path doesn't have a callback, or it is nil, we behave as before

Since rds.Instance is currently the only resource with an explicit callback, could there ever be a new PreCheckCallback that doesn't want this to happen?

Yeah this just looks like "add a callback instead of replacing it".

If there is a callback, we give re-assign prov.Resources[key].PreCheckCallback so both the old callback and applyTags are called. Otherwise we just set applyTags.

If we introduce a new callback where we don't want this to happen, we can change this code. I don't expect that we will though.

If there is a callback, we give re-assign prov.Resources[key].PreCheckCallback so both the old callback and applyTags are called. Otherwise we just set applyTags.

Makes sense!

If we introduce a new callback where we don't want this to happen, we can change this code. I don't expect that we will though.

How will a future maintainer know to change this code?
If we don't expect new callbacks to be introduced - should we use the resource name as an explicit trigger here?

How will a future maintainer know to change this code?

If a future maintainer wants to add a new PreCheckCallback to a specific resource that overrides the behavior of tags, they will notice their change is not working and investigate. It's all in this file. They can grep for PreCheckCallback in resources.go and find this.

If we don't expect new callbacks to be introduced - should we use the resource name as an explicit trigger here?

I'm not sure what you mean? Can you elaborate?

they will notice their change is not working and investigate.

I don't think that is ideal. I think the design of PreCheckCallback invites one to think "oh neat, I can add a callback in this place, on a resource". Getting a surprise can be really frustrating, regardless of how easy it would be to grep for PreCheckCallback.

Can you elaborate?

If this particular callback is unique to rds.Instance, I think I would prefer logic that explicitly triggers on "if this resource is rds.Instance, use this callback". That will avoid a future maintainer having to look why something seemingly unrelated is getting involved with their callback.

I guess my TL;DR is: I don't think that "PreCheckCallback is not nil" is a discerning enough evaluation to decide whether we should call this code. Let's just use rds.Instance. After all, we would want to remove this on v7, correct?

iwahbe · 2023-08-09T13:28:10Z

We can get the warning we want easily after we merge pulumi/pulumi-terraform-bridge#1333.

provider/resources.go

t0yv0 · 2023-08-09T13:30:06Z

provider/resources.go

+							// up in the schema. It is never observed non-nil by the
+							// upstream provider and we warn when you set it.
+
+							p.ResourcesMap().Get("aws_db_instance").Schema().


This is a little too interesting, you prefer this to having the upstream patch?

Yes, because:

The totality of the change is well defined.

Its obvious to maintainers that it is a Pulumi change.

The code is next to PreCheckCallback which is it logically tied to.

There cannot be merge conflicts.

t0yv0 · 2023-08-09T13:30:19Z

provider/resources.go

+						// Name doesn't actually exist on the underlying provider anymore,
+						// so we make sure it only sees `dbName`, not `name`.
+						config["dbName"] = name
+						delete(config, "name")


provider/cmd/pulumi-resource-aws/schema.json

t0yv0 · 2023-08-09T13:32:34Z

sdk/dotnet/Rds/Instance.cs

@@ -489,7 +489,7 @@ public partial class Instance : global::Pulumi.CustomResource
        public Output<bool> MultiAz { get; private set; } = null!;

        [Output("name")]
-        public Output<string> Name { get; private set; } = null!;
+        public Output<string?> Name { get; private set; } = null!;


Interesting. Benign? This might be coming out of tfgen changes actually

This is real and correct. name is not set if the user correctly supplies dbName.

t0yv0 · 2023-08-09T13:34:10Z

upstream-patches/0015-Un-deprecate-RDS-instance-name.patch

- 				Optional: true,
- 				Computed: true,
- 			},
-+			"name": {


Yes! Here we go, delete this one.

t0yv0

A few nits but 🚀 🚀 this looks just like what we want.

Any chance to attach how Pulumi responds to the failing example e.g. how the deprecation message looks like? Thank you!

netum-ArturMalinin · 2023-09-21T09:41:55Z

Hello, I noticed this PR as we are facing issues when upgrading from version 5.42.0 to 6.2.0 with Python.

Firstly, this warning is probably incorrect:
warning: name is deprecated: This property has been deprecated. Please use 'dbName' instead.

As there is no 'dbName' property here: https://github.com/pulumi/pulumi-aws/blob/master/sdk/python/pulumi_aws/rds/instance.py#L19-L81

Pulumi is also attempting to execute a replacement of the aws.rds.Instance in our case. This is probably not as intended?

When using the deprecated name property,Pulumi preview shows:
aws:rds:Instance **-*****-*** replace [diff: +dbName]; 2 warnings

If we change the name property to db_name, Pulumi preview displays:
aws:rds:Instance **-*****-*** replace [diff: +dbName]

If we don't set properties name or db_name Pulumi preview runs without any changes (update/replace) to the RDS instance.

t0yv0 · 2023-09-21T12:57:13Z

I'm sorry you are running into this. Could you please open a new issue, this will help my team prioritize looking at this! Thank you!

Remove name from upstream

f220cb3

iwahbe self-assigned this Aug 9, 2023

guineveresaenger reviewed Aug 9, 2023

View reviewed changes

t0yv0 reviewed Aug 9, 2023

View reviewed changes

provider/resources.go Show resolved Hide resolved

t0yv0 reviewed Aug 9, 2023

View reviewed changes

provider/cmd/pulumi-resource-aws/schema.json Show resolved Hide resolved

t0yv0 reviewed Aug 9, 2023

View reviewed changes

t0yv0 approved these changes Aug 9, 2023

View reviewed changes

t0yv0 mentioned this pull request Aug 9, 2023

Fix RDS instance name problem #2685

Closed

iwahbe force-pushed the iwahbe/2682/depricate-name branch from 08fde9b to 041c604 Compare August 9, 2023 13:41

iwahbe added 2 commits August 9, 2023 16:00

Deprecate 'name' in favor of 'dbName'

81ed7eb

Add a test

e7a9a30

iwahbe force-pushed the iwahbe/2682/depricate-name branch from 041c604 to e7a9a30 Compare August 9, 2023 14:01

iwahbe and others added 3 commits August 11, 2023 19:13

add deprication message and update bridge

bac376e

go mod tidy

0669030

Merge branch 'master' into iwahbe/2682/depricate-name

b85452a

t0yv0 enabled auto-merge (squash) August 11, 2023 22:24

t0yv0 disabled auto-merge August 11, 2023 22:24

t0yv0 merged commit f158e81 into master Aug 11, 2023
16 checks passed

t0yv0 deleted the iwahbe/2682/depricate-name branch August 11, 2023 22:25

iwahbe mentioned this pull request Aug 24, 2023

Keep httpEndpoint valid pulumi/pulumi-confluentcloud#185

Merged

netum-ArturMalinin mentioned this pull request Sep 22, 2023

aws.rds.Instance replacement when upgrading from 5.42.0 to 6.2.0 #2823

Closed

renovate bot mentioned this pull request May 10, 2024

Update aws packages (major) jonashackt/pulumi-typescript-aws-fargate#12

Open

1 task

t0yv0 mentioned this pull request Jun 4, 2024

DO NOT MERGE: test plan resource change with the aws test suite #4013

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `name` on `aws.rds.Instance` #2686

Deprecate `name` on `aws.rds.Instance` #2686

iwahbe commented Aug 9, 2023 •

edited

github-actions bot commented Aug 9, 2023 •

edited

guineveresaenger Aug 9, 2023 •

edited

t0yv0 Aug 9, 2023

iwahbe Aug 9, 2023

guineveresaenger Aug 10, 2023 •

edited

iwahbe Aug 10, 2023

guineveresaenger Aug 11, 2023 •

edited

iwahbe commented Aug 9, 2023

t0yv0 Aug 9, 2023

iwahbe Aug 9, 2023

t0yv0 Aug 9, 2023

t0yv0 Aug 9, 2023

iwahbe Aug 9, 2023

t0yv0 Aug 9, 2023

t0yv0 left a comment

netum-ArturMalinin commented Sep 21, 2023

t0yv0 commented Sep 21, 2023

Deprecate name on aws.rds.Instance #2686

Deprecate name on aws.rds.Instance #2686

Conversation

iwahbe commented Aug 9, 2023 • edited

github-actions bot commented Aug 9, 2023 • edited

Does the PR have any schema changes?

Resources

guineveresaenger Aug 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guineveresaenger Aug 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guineveresaenger Aug 11, 2023 • edited

Choose a reason for hiding this comment

iwahbe commented Aug 9, 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

t0yv0 left a comment

Choose a reason for hiding this comment

netum-ArturMalinin commented Sep 21, 2023

t0yv0 commented Sep 21, 2023

Deprecate `name` on `aws.rds.Instance` #2686

Deprecate `name` on `aws.rds.Instance` #2686

iwahbe commented Aug 9, 2023 •

edited

github-actions bot commented Aug 9, 2023 •

edited

guineveresaenger Aug 9, 2023 •

edited

guineveresaenger Aug 10, 2023 •

edited

guineveresaenger Aug 11, 2023 •

edited