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

Bug: Create own struct to be able to add omitempty json tag on attributes #35950

Merged

Conversation

stromp
Copy link
Contributor

@stromp stromp commented Feb 23, 2024

Description

The whole struct was returned which marshalled attributes with a null values. Those attributes should be omitted, since you can not use them when you provider the json to AWS.

Relations

Relates #33565

Output from Acceptance Testing

=== RUN TestAccSSMPatchBaseline_basic
=== PAUSE TestAccSSMPatchBaseline_basic
=== CONT TestAccSSMPatchBaseline_basic
--- PASS: TestAccSSMPatchBaseline_basic (34.30s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/ssm 34.398s
...

Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/ssm Issues and PRs that pertain to the ssm service. labels Feb 23, 2024
@terraform-aws-provider terraform-aws-provider bot added the needs-triage Waiting for first response or review from a maintainer. label Feb 23, 2024
@stromp stromp force-pushed the b-filter_null_values_so_AWS_does_not_complain branch from 6f1cbfe to 571736d Compare February 26, 2024 09:49
@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 27, 2024
@justinretzolk
Copy link
Member

Hey @stromp 👋 Thank you for your contribution here! I'm not one of the people who will be doing a final review on this, but I wanted to note that I added the enhancement label rather than the bug one, since this is a change to make the resource more friendly to use, and not something that causes the provider to throw errors. If I've misunderstood, please do let me know and we can update the labels.

@stromp
Copy link
Contributor Author

stromp commented Feb 29, 2024

Hey @stromp 👋 Thank you for your contribution here! I'm not one of the people who will be doing a final review on this, but I wanted to note that I added the enhancement label rather than the bug one, since this is a change to make the resource more friendly to use, and not something that causes the provider to throw errors. If I've misunderstood, please do let me know and we can update the labels.

hi @justinretzolk.

I guess it is a bit how you look at it. Currently the resource will give you a different JSON result then the original JSON returned by the AWS API would. Since the purpose of the attribute is that it returns the API JSON I would personally classify this as a bug.

@stromp stromp force-pushed the b-filter_null_values_so_AWS_does_not_complain branch from 571736d to f43bd49 Compare May 27, 2024 13:35
@stromp
Copy link
Contributor Author

stromp commented May 27, 2024

Hey @stromp 👋 Thank you for your contribution here! I'm not one of the people who will be doing a final review on this, but I wanted to note that I added the enhancement label rather than the bug one, since this is a change to make the resource more friendly to use, and not something that causes the provider to throw errors. If I've misunderstood, please do let me know and we can update the labels.

hi @justinretzolk.

I guess it is a bit how you look at it. Currently the resource will give you a different JSON result then the original JSON returned by the AWS API would. Since the purpose of the attribute is that it returns the API JSON I would personally classify this as a bug.

hi @justinretzolk

I have just rebased the branch since it became out of sync due to the aws sdk update. Can I ask for some attention to get this merged?

@stromp stromp force-pushed the b-filter_null_values_so_AWS_does_not_complain branch from 8b858eb to a9e5047 Compare June 6, 2024 09:45
stromp and others added 3 commits June 6, 2024 12:33
This generic helper function strips nil values, empty arrays, and empty objects from the provided byte slice, preventing nil values from the AWS API response from being stored in the computed `json` attribute.
… case

This test verifies that nil values returned in the ApprovalRule response are appropriately scrubbed from the `json` attribute.

```console
% make testacc PKG=ssm TESTS=TestAccSSMPatchBaseline_
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.2 test ./internal/service/ssm/... -v -count 1 -parallel 20 -run='TestAccSSMPatchBaseline_'  -timeout 360m

--- PASS: TestAccSSMPatchBaseline_disappears (31.31s)
=== CONT  TestAccSSMPatchBaseline_tags_null
--- PASS: TestAccSSMPatchBaseline_rejectPatchesAction (37.77s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_providerOnly
--- PASS: TestAccSSMPatchBaseline_approvedPatchesNonSec (39.94s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_updateToProviderOnly
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nullOverlappingResourceTag (43.85s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_updateToResourceOnly
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nullNonOverlappingResourceTag (43.91s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_overlapping
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_emptyProviderOnlyTag (45.51s)
=== CONT  TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Replace
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnCreate (49.76s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_emptyResourceTag (50.56s)
--- PASS: TestAccSSMPatchBaseline_operatingSystem (56.32s)
--- PASS: TestAccSSMPatchBaseline_sources (59.38s)
--- PASS: TestAccSSMPatchBaseline_approveUntilDateParam (59.69s)
--- PASS: TestAccSSMPatchBaseline_approvalRuleEmpty (67.48s)
--- PASS: TestAccSSMPatchBaseline_basic (67.74s)
--- PASS: TestAccSSMPatchBaseline_tags_AddOnUpdate (68.98s)
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnUpdate_Replace (70.36s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnCreate (72.92s)
--- PASS: TestAccSSMPatchBaseline_tags_null (42.16s)
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnUpdate_Add (75.00s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Add (85.97s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_updateToResourceOnly (42.60s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_updateToProviderOnly (46.77s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Replace (41.84s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nonOverlapping (89.47s)
--- PASS: TestAccSSMPatchBaseline_tags (100.06s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_overlapping (59.82s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_providerOnly (77.53s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ssm        121.254s
```
@jar-b jar-b requested a review from a team as a code owner July 30, 2024 15:12
Copy link
Member

@jar-b jar-b left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

% make testacc PKG=ssm TESTS=TestAccSSMPatchBaseline_
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.2 test ./internal/service/ssm/... -v -count 1 -parallel 20 -run='TestAccSSMPatchBaseline_'  -timeout 360m

--- PASS: TestAccSSMPatchBaseline_disappears (31.31s)
=== CONT  TestAccSSMPatchBaseline_tags_null
--- PASS: TestAccSSMPatchBaseline_rejectPatchesAction (37.77s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_providerOnly
--- PASS: TestAccSSMPatchBaseline_approvedPatchesNonSec (39.94s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_updateToProviderOnly
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nullOverlappingResourceTag (43.85s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_updateToResourceOnly
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nullNonOverlappingResourceTag (43.91s)
=== CONT  TestAccSSMPatchBaseline_tags_DefaultTags_overlapping
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_emptyProviderOnlyTag (45.51s)
=== CONT  TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Replace
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnCreate (49.76s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_emptyResourceTag (50.56s)
--- PASS: TestAccSSMPatchBaseline_operatingSystem (56.32s)
--- PASS: TestAccSSMPatchBaseline_sources (59.38s)
--- PASS: TestAccSSMPatchBaseline_approveUntilDateParam (59.69s)
--- PASS: TestAccSSMPatchBaseline_approvalRuleEmpty (67.48s)
--- PASS: TestAccSSMPatchBaseline_basic (67.74s)
--- PASS: TestAccSSMPatchBaseline_tags_AddOnUpdate (68.98s)
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnUpdate_Replace (70.36s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnCreate (72.92s)
--- PASS: TestAccSSMPatchBaseline_tags_null (42.16s)
--- PASS: TestAccSSMPatchBaseline_tags_ComputedTag_OnUpdate_Add (75.00s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Add (85.97s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_updateToResourceOnly (42.60s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_updateToProviderOnly (46.77s)
--- PASS: TestAccSSMPatchBaseline_tags_EmptyTag_OnUpdate_Replace (41.84s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_nonOverlapping (89.47s)
--- PASS: TestAccSSMPatchBaseline_tags (100.06s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_overlapping (59.82s)
--- PASS: TestAccSSMPatchBaseline_tags_DefaultTags_providerOnly (77.53s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ssm        121.254s

@jar-b
Copy link
Member

jar-b commented Jul 30, 2024

Thanks for your contribution, @stromp! 👍

@jar-b jar-b merged commit 9503f32 into hashicorp:main Jul 30, 2024
34 checks passed
@github-actions github-actions bot added this to the v5.61.0 milestone Jul 30, 2024
Copy link

github-actions bot commented Aug 2, 2024

This functionality has been released in v5.61.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Copy link

github-actions bot commented Sep 1, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ssm Issues and PRs that pertain to the ssm service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants