Skip to content
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

Don't send writeOnly properties if they're also createOnly #1448

Merged
merged 10 commits into from
Mar 22, 2024

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Mar 20, 2024

Fixes #1435

Ideally we'd have an integration test around this case to avoid future regression but currently the VPC-based test is very unstable, so has been disabled, but included for documentation of the issues.

  • Deletion of the aws-native:ec2:IpamPoolCidr regularly fails with "GeneralServiceException": Error occurred during operation 'The CIDR has one or more allocations.' even after the VPC is deleted for serveral minutes.
  • Sometimes Update of the aws-native:ec2:IpamPool fails with "NotStabilized": IpamPoolCidrFailureReason(Message=The CIDR has one or more allocations.
  • The refresh of the aws-native:ec2:IpamPool fails due to a diff in the provisionedCidrs property.
  • It's also not ideal that we have to create the IPAM manually to avoid the test failing if run in parallel as only 1 IPAM is allowed to be created in each region.

Extracted the patch generation code instead and wrote a unit around it. Also replicated the fix into the untyped ExtensionResource implementation and tested using the same test suite.

@danielrbradley danielrbradley self-assigned this Mar 20, 2024
@danielrbradley danielrbradley added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 20, 2024
Copy link
Contributor

Does the PR have any schema changes?

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

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 66 lines in your changes are missing coverage. Please review.

Project coverage is 24.60%. Comparing base (64dbca9) to head (64e78dc).
Report is 1 commits behind head on master.

Files Patch % Lines
provider/pkg/resources/extension_resource.go 0.00% 58 Missing ⚠️
provider/pkg/resources/patching.go 84.44% 4 Missing and 3 partials ⚠️
provider/pkg/provider/provider.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   23.72%   24.60%   +0.87%     
==========================================
  Files          31       32       +1     
  Lines        4446     4475      +29     
==========================================
+ Hits         1055     1101      +46     
+ Misses       3227     3208      -19     
- Partials      164      166       +2     

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

This test _can_ be run manually to prove the change works as expected, but the resources involved make this unfeasible to enable right now as an automated test.
Copy link
Member

@mikhailshilkov mikhailshilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change makes sense to me, but I’m missing a unit test with it

- Inline schema property specs.
- Add exclusion of create-only properties when appending to the patch.
- Re-purpose existing tests to run against both implementations.
- Fix leading "/" in write-only property paths.
Make the outputs of typed and untyped diffing exactly the same (apart from case conversion).
Copy link
Member

@mjeffryes mjeffryes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you were able to bring the patch logic for extension and other resources together with this change.

I did have on significant concern about the introduction of a createOnly property on the ExtensionResource, but otherwise just a few nits.

provider/pkg/naming/convert.go Outdated Show resolved Hide resolved
provider/pkg/provider/provider.go Outdated Show resolved Hide resolved
provider/pkg/resources/extension_resource.go Outdated Show resolved Hide resolved
provider/pkg/resources/patching.go Outdated Show resolved Hide resolved
provider/pkg/resources/patching.go Show resolved Hide resolved
- Improve comments & documentation.
- Tweak how we do sorting of keys.
@danielrbradley danielrbradley force-pushed the 1435-createonly-writeonly branch from ec8345a to 64e78dc Compare March 22, 2024 20:11
@danielrbradley danielrbradley merged commit 210092f into master Mar 22, 2024
17 checks passed
@danielrbradley danielrbradley deleted the 1435-createonly-writeonly branch March 22, 2024 20:40
@arianvp
Copy link

arianvp commented Mar 23, 2024

Deletion of the aws-native:ec2:IpamPoolCidr regularly fails with "GeneralServiceException": Error occurred during operation 'The CIDR has one or more allocations.' even after the VPC is deleted for serveral minutes.

Yeh it takes very long for the IpamPoolCidr to be released back into the pool unfortunately :( I guess this is something for Amazon to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not update Vpc if using ipv4IpamPoolId and ipv4NetmaskLength
4 participants