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

Add pagination support (introduced in aws-sdk-go v1.33.21) and multiple baselines per patch group. #15213

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

melbit-michaelw
Copy link
Contributor

This PR modifies the Id for an SSM Patch Group to be the Patch Group Name joined to the BaselineId by a ":". This allows an SSM Patch Group to support multiple baselines.

Please note that pre-existing patch groups will be recreated due to the change in Id (this happens without issue in my testing as the AWS API call succeeds even if the patch group already has that baseline applied).

Secondly, this PR adds pagination support (added in aws-sdk-go v1.33.21) to the aws_ssm_patch_group resource.

Community Note

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

Closes #9603
Closes #13580

As it involved modifying the same code, I have merged the work from my previous PR ( #13752 ) into this one. Please let me know if that's a problem..

Release note for CHANGELOG:

resource/aws_ssm_patch_group: Support multiple baselines (one per OS) on an SSM patch group (#9603)
resource/aws_ssm_patch_group: Support pagination for patch groups (#13580)

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSSMPatchGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSSMPatchGroup -timeout 120m
=== RUN   TestAccAWSSSMPatchGroup_basic
=== PAUSE TestAccAWSSSMPatchGroup_basic
=== CONT  TestAccAWSSSMPatchGroup_basic
--- PASS: TestAccAWSSSMPatchGroup_basic (28.19s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       29.634s

@melbit-michaelw melbit-michaelw requested a review from a team September 17, 2020 23:05
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/ssm Issues and PRs that pertain to the ssm service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. needs-triage Waiting for first response or review from a maintainer. labels Sep 17, 2020
@melbit-michaelw
Copy link
Contributor Author

Is there anything I can do to get this PR looked at ?

@melbit-michaelw
Copy link
Contributor Author

@YakDriver , @gdavison - Apologies for reaching out to you both directly, but this PR has been pending review for quite some time now.

It is functionality that my company urgently needs.

Is there anything further I need to do (or can do) in order to get it reviewed ?

Thank you for your time.

@fatalglitch
Copy link

This seems like it has been pending for a very long time, with no issues identified. When can this get merged?

@melbit-michaelw
Copy link
Contributor Author

@apparentlymart , @YakDriver , @gdavison , @anGie44 - Apologies for mentioning you all directly, but I don't know what else I can do to get this PR reviewed..

It's been waiting for over 2 months now with absolutely no feedback.

Is there anything further I need to do (or can do) in order to get it reviewed and progressing ?

Thank you for your time.

@breathingdust
Copy link
Member

👋 Hi @melbit-michaelw thanks for the contribution! Apologies for the radio silence, the engineers get so many direct and indirect (via team) pings its just not feasible to answer every one. Right now the team are working through items in order of priority, primarily determined by upvotes on the PR or linked issue. I can see that this issue/pr has significant support, so it is is in our backlog but at this time we can't give an ETA for when it will be reviewed.

We appreciate your patience and thanks again for the contribution!

Base automatically changed from master to main January 23, 2021 00:59
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:59
@eric51893
Copy link

Hello, been a while, any more clarity on the status of this request?

@anGie44 anGie44 added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 17, 2021
@anGie44 anGie44 self-assigned this Mar 17, 2021
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

Hi @melbit-michaelw, thank you for this PR! Overall the resource ID change and pagination support are great solutions to the associated bug. Just a couple quick notes that I'll be pushing up changes to address so we can get this into the upcoming release.

  1. As you mentioned, we'll need a state migration to ensure backwards compatability, so I've added functionality and a test.
  2. The AWS docs list : as a valid character for both resource arguments, so later when we parse the ID based on the delimiter it could become troublesome, so instead I've replaced the delimiter with a ,.
  3. Since the pagination function is used across the resource and test file, I've moved it to the ssm internal service package for a more DRY implementation.
  4. When iterating through the PatchGroupsPages, I've preemptively parsed the ID if in the future we support importing this resource.
  5. I've added test coverage for the 1:many relationship

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 17, 2021
@anGie44
Copy link
Contributor

anGie44 commented Mar 17, 2021

Output of acceptance tests (commercial):

--- PASS: TestAccAWSSSMPatchGroup_basic (14.19s)
--- PASS: TestAccAWSSSMPatchGroup_disappears (15.23s)
--- PASS: TestAccAWSSSMPatchGroup_multipleBaselines (15.80s)

--- PASS: TestResourceAWSSSMPatchGroupStateUpgradeV0 (0.00s)

@anGie44 anGie44 added this to the v3.33.0 milestone Mar 17, 2021
@anGie44 anGie44 force-pushed the b-patch-group-pagination branch 2 times, most recently from 123ef6c to aeeb20d Compare March 17, 2021 14:46
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

Thank you again @melbit-michaelw !🚀

@anGie44 anGie44 merged commit e2dde22 into hashicorp:main Mar 17, 2021
@ghost
Copy link

ghost commented Mar 18, 2021

This has been released in version 3.33.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 for triage. Thanks!

@ghost
Copy link

ghost commented Apr 16, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. service/ssm Issues and PRs that pertain to the ssm service. size/XL 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
5 participants