-
Notifications
You must be signed in to change notification settings - Fork 167
[7.0.0-alpha]: Upgrade upstream to v6.0.0-beta1 #5511
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
Conversation
This change is part of the following stack: Change managed by git-spice. |
d1c99a6
to
cc11ec3
Compare
cc11ec3
to
923da17
Compare
Does the PR have any schema changes?Found 97 breaking changes: Resources
Functions
Types
New resources:
Maintainer note: consult the runbook for dealing with any breaking changes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 7.0.0-alpha #5511 +/- ##
==============================================
Coverage ? 24.43%
==============================================
Files ? 360
Lines ? 143138
Branches ? 0
==============================================
Hits ? 34969
Misses ? 108069
Partials ? 100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
provider/diff_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created follow up to re-enable.
#5518
@@ -1932,6 +1932,9 @@ compatibility shim in favor of the new "name" field.`) | |||
"aws_internet_gateway_attachment": {Tok: awsResource(ec2Mod, "InternetGatewayAttachment")}, | |||
"aws_ec2_image_block_public_access": {Tok: awsResource(ec2Mod, "ImageBlockPublicAccess")}, | |||
"aws_ec2_instance_metadata_defaults": {Tok: awsResource(ec2Mod, "InstanceMetadataDefaults")}, | |||
"aws_ec2_default_credit_specification": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from master
. These updates were included in both v5 & v6-beta
0acf7d5
to
2282852
Compare
2282852
to
dcb6976
Compare
@@ -67,10 +67,6 @@ func TestAccPluginFramework(t *testing.T) { | |||
integration.ProgramTest(t, &test) | |||
} | |||
|
|||
func TestBucketUpgrade(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have much more robust bucket upgrade tests now. Removing this simple one.
@@ -394,85 +372,6 @@ func TestNonIdempotentSnsTopic(t *testing.T) { | |||
require.ErrorContains(t, err, "already exists") | |||
} | |||
|
|||
func TestOpenZfsFileSystemUpgrade(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was specific to testing the removal of MaxItemsOne during a minor version update and testing we could go from a string to a list. This isn't relevant anymore now that we are going from v6 to v7.
// | ||
// Updated in https://github.com/pulumi/pulumi-aws/pull/3881 | ||
// replacements. | ||
func TestMigrateRdsInstance(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this since it tested an issue with going from v5 to v6. Not relevant anymore for v6 to v7.
if isRegionOverrideEnabled && getAttribute != nil { | ||
if region, ok := getAttribute(names.AttrRegion); ok { | ||
- overrideRegion = region.(string) | ||
+ if val, ok := region.(string); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: once merged create tracking issue. Maybe a bridge issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's overrideRegion
prior to this line? The remaining code is in another patch, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the code is just upstream code. The only patch we have in this code is what i've added here.
The line above which isn't included in the patch just initializes overrideRegion.
var overrideRegion string
getAttribute
is a function looks up the attribute in the rawState, essentially something like this:
getAttribute := func(key: string): (any, bool) {
v, ok := rawState[key]; return v, ok
}
In Terraform it looks like at the point this code is running rawState
does not have a region
attribute at all so getAttribute
returns false
. In Pulumi for some reason rawState
does have a region
attribute with a nil
value. I think it's a bridge issue so I'll create a follow up issue.
Add new patch for region issue
- Adds a `PreCheckCallback` for the `region` property similar to how we handle `tagsAll` - Because of this I also refactored out the `tagsAll` handling into a separate function. - Added missing upstream resources added in latest release - Ran `make tfgen`
ad45f4e
to
e34bb4e
Compare
if isRegionOverrideEnabled && getAttribute != nil { | ||
if region, ok := getAttribute(names.AttrRegion); ok { | ||
- overrideRegion = region.(string) | ||
+ if val, ok := region.(string); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's overrideRegion
prior to this line? The remaining code is in another patch, right?
// doesn't work with the way the bridge/pulumi works. The interceptor will add the region after | ||
// Pulumi runs `Check` which causes issues. This workaround sets the `region` the same way upstream will | ||
// but does it on the pulumi side before `Check` | ||
// TODO[pulumi/pulumi-aws#5521] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we just consider these precheck callbacks a temporary workaround then? What would a permanent solution look like? Would the suggestion here be to align Pulumi with TF where refresh
behavior is concerned?
I suppose my question boils down to: why would we still be seeing a Region diff when we're setting it here before Check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two separate issues here that are both at play.
- In Pulumi all input properties have to be set in
Check
. If input properties are added afterCheck
then it causes weird diff issues. Terraform is setting theregion
input property in an interceptor that runs for us afterCheck
. ThePreCheckCallback
is a way for us to make sure theregion
is set prior toCheck
so the diff shows correctly. Related to Consider moving planning to be performed in Check pulumi-terraform-bridge#2281
For example, by default we get a diff like this (because we didn't set the region value in Check
)
}
~ aws:s3/bucketOwnershipControls:BucketOwnershipControls: (update)
[id=loggingbucket-0c177aa]
[urn=urn:pulumi:chall::test-aws-bucket-migration::aws:s3/bucketOwnershipControls:BucketOwnershipControls::exampleBucketOwnershipControls]
[provider=urn:pulumi:chall::test-aws-bucket-migration::pulumi:providers:aws::provider::2205ad1c-2cf0-4b64-9bd2-3fa6b2c1083b]
+ region: <null>
Adding the PreCheckCallback
leads to a diff like this (better)
}
~ aws:s3/bucketOwnershipControls:BucketOwnershipControls: (update)
[id=loggingbucket-0c177aa]
[urn=urn:pulumi:chall::test-aws-bucket-migration::aws:s3/bucketOwnershipControls:BucketOwnershipControls::exampleBucketOwnershipControls]
[provider=urn:pulumi:chall::test-aws-bucket-migration::pulumi:providers:aws::provider::2205ad1c-2cf0-4b64-9bd2-3fa6b2c1083b]
+ region: "us-east-2"
- The second issue is that we shouldn't see the diff at all since Terraform doesn't show a diff in this case. Terraform also sets the
region
in a interceptor that runs afterRead
which means when diff runs it has the same value in the old and new inputs. So if we runup --refresh
thenRead
runs and that post-read interceptor runs and we don't see the diff.
@@ -595,6 +595,9 @@ func TestRegress2868(t *testing.T) { | |||
test := getJSBaseOptions(t). | |||
With(integration.ProgramTestOptions{ | |||
Dir: filepath.Join(getCwd(t), "regress-2868"), | |||
// TODO[pulumi/pulumi-aws#5521] `region` causes permanent diff without refresh | |||
PreviewCommandlineFlags: []string{"--refresh"}, | |||
UpdateCommandlineFlags: []string{"--refresh"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above: does the PreCheckCallback not apply here?
Upgrade upstream to v6.0.0-beta1
This PR upgrades upstream to the new v6.0.0-beta1 tag
The major update to the SDKs in this update is the addition of the new
region
resource level propertyHow to review
Contains the changes to patches. Pretty minor changes
This is the bulk of the changes I made in this commit
Contains all the updates I had to make to the tests. I also regenerated all of our upgrade tests so they are testing the upgrade from v6 => v7.
The SDK changes are all in a single commit. Pretty much every resource is updated, but almost all the changes are related to adding the region parameter.
re #5226