-
Notifications
You must be signed in to change notification settings - Fork 158
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
Upstream 5.85.0 #5161
Upstream 5.85.0 #5161
Conversation
This change is part of the following stack: Change managed by git-spice. |
3bdc626
to
94c0821
Compare
@@ -48,8 +48,8 @@ index 57bf4c7c94..09b4279db3 100644 | |||
+ } | |||
+ } | |||
+ | |||
+ sp, ok := c.ServicePackages[servicePackageName] | |||
+ if !ok { | |||
+ sp := c.ServicePackage(ctx, servicePackageName) |
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.
c.ServicePackages
was changed to a private type with accessor methods.
prov.Resources["aws_guardduty_member_detector_feature"].ComputeID = func( | ||
ctx context.Context, state resource.PropertyMap, | ||
) (resource.ID, error) { | ||
return attrWithSeparator(state, ",", "detectorId", "accountId", "name"), nil |
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 resource currently doesn't support import, so I'm not 100% sure this is correct. Based on the Read
method it looks like these three are required
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.
Ah yes. The ID allocation is best-effort. Technically new GUID is a valid ID. We've been trying to match ID to import IDs where possible.
@@ -4666,6 +4668,8 @@ compatibility shim in favor of the new "name" field.`) | |||
"aws_ec2_managed_prefix_list": {Tok: awsDataSource(ec2Mod, "getManagedPrefixList")}, | |||
"aws_ec2_transit_gateway_route_tables": {Tok: awsDataSource(ec2Mod, "getTransitGatewayRouteTables")}, | |||
"aws_ec2_instance_types": {Tok: awsDataSource(ec2Mod, "getInstanceTypes")}, | |||
"aws_vpc_ipam": {Tok: awsDataSource(ec2Mod, "getVpcIpam")}, |
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 is aws_vpc_*
, but everything else we've been putting in aws_ec2
so just following that.
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.
Which module does it show up in the TF documentation site? Alongside which resources? Can we match that for minimum surprise?
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.
Well the problem is that we haven't been matching TF so far. The TF docs site has a separate section for Vpc
(which includes a lot more resources that our Vpc docs section) and a separate Vpc IPAM
section.
It seems like we've already diverged pretty far from what the TF docs do, but that also might be because they are able to re-organize the resources on the docs site when they want to.
Also, it might be a moot point because I didn't realize we already had a aws_vpc_ipam
resource
that we put under the ec2
module so I think we need to also put the datasource there as well.
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.
I'd like to maybe clean this up. WDYT? Aliasing may have our back but would like to move these resources into their new home module that matches TF upstream. Perhaps we can take a task into V7 planning to finish this up?
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.
That's a good idea, we should think about how to reorganize these resources for V7. I'll create an issue to track.
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.
8aea02c
to
d03c0dd
Compare
d03c0dd
to
09b8261
Compare
lambda.WithEndpointResolverV2(newEndpointResolverV2()), | ||
withBaseEndpoint(config[names.AttrEndpoint].(string)), | ||
withExtraOptions(ctx, p, config), | ||
+ p.pulumiCustomizeLambdaRetries(cfg), |
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.
Anything special going on here?
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.
Looks like they added the withExtraOptions
thing in there so I just fixed the rebase to add our customization after.
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.
PATCH changes LGTM. Let me know if need a hand fighting CI.
Does the PR have any schema changes?Found 2 breaking changes: Resources
Types
New resources:
New functions:
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 @@
## master #5161 +/- ##
==========================================
+ Coverage 24.47% 24.54% +0.06%
==========================================
Files 361 362 +1
Lines 144065 144734 +669
==========================================
+ Hits 35260 35524 +264
- Misses 108707 109112 +405
Partials 98 98 ☔ View full report in Codecov by Sentry. |
7b54123
to
b96354c
Compare
This PR has been shipped in release v6.68.0. |
Upgrade terraform-provider-aws to v5.85.0