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.

Sign up for GitHub

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

Jump to bottom

r/secretsmanager_secret_policy - new resource #14468

Merged
merged 14 commits into from
Nov 2, 2020
Merged

r/secretsmanager_secret_policy - new resource #14468

merged 14 commits into from
Nov 2, 2020

Conversation

DrFaust92
Copy link
Collaborator

@DrFaust92 DrFaust92 commented Aug 4, 2020 •
edited
Loading

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 #14618
Release note for CHANGELOG:

resource_aws_secretsmanager_secret_policy - new resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsSecretsManagerSecretPolicy_'
--- PASS: TestAccAwsSecretsManagerSecretPolicy_basic (92.55s)
--- PASS: TestAccAwsSecretsManagerSecretPolicy_blockPublicPolicy (122.51s)
--- PASS: TestAccAwsSecretsManagerSecretPolicy_disappears (48.91s)

@DrFaust92 DrFaust92 requested a review from a team August 4, 2020 19:02
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/secretsmanager Issues and PRs that pertain to the secretsmanager service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Aug 4, 2020
@DrFaust92 DrFaust92 added the new-resource Introduces a new resource. label Aug 4, 2020
@DrFaust92 DrFaust92 mentioned this pull request Aug 13, 2020
Closed
@DrFaust92 DrFaust92 force-pushed the r/secretsmanager_secret_policy branch from b9c11da to 230324c Compare August 13, 2020 19:03
@DrFaust92
Copy link
Collaborator Author

Rebased + added support for blocking public access:

--- PASS: TestAccAwsSecretsManagerSecretPolicy_basic (92.55s)
--- PASS: TestAccAwsSecretsManagerSecretPolicy_blockPublicPolicy (122.51s)
--- PASS: TestAccAwsSecretsManagerSecretPolicy_disappears (48.91s)

@DrFaust92 DrFaust92 force-pushed the r/secretsmanager_secret_policy branch from e69fffb to 5db1047 Compare August 22, 2020 10:56
@haidaraM
Copy link
Contributor

haidaraM commented Oct 6, 2020

Hi,
Thanks for this PR :-) Any idea of when this will be merged.

@haidaraM haidaraM mentioned this pull request Oct 9, 2020
Merged
@dblooman
Copy link
Contributor

Can we get an update on this PR please 👍

@bill-rich bill-rich self-assigned this Oct 23, 2020
bill-rich
bill-rich reviewed Oct 28, 2020
View reviewed changes
Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

Looks great overall. I had about one of the conditions that came up twice. Please let me know on those.

aws/resource_aws_secretsmanager_secret_policy.go
return nil
})
if isResourceTimeoutError(err) {
res, err = conn.PutResourcePolicy(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the policy being applied again if the previous attempt timed out? Is this an expected condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bill-rich this is required in the AWS Provider due to how the AWS Go SDK works, see also:

aws/resource_aws_secretsmanager_secret_policy.go
return nil
})
if isResourceTimeoutError(err) {
_, err = conn.PutResourcePolicy(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@DrFaust92 DrFaust92 force-pushed the r/secretsmanager_secret_policy branch from 5db1047 to 91e4655 Compare October 29, 2020 11:26
@DrFaust92
Copy link
Collaborator Author

Rebased for acceptance test config lint

bill-rich
bill-rich approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

I got my question answered. LGTM

@bill-rich bill-rich merged commit 0788cd2 into hashicorp:master Nov 2, 2020
@ghost
Copy link

ghost commented Dec 3, 2020

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 Dec 3, 2020
@DrFaust92 DrFaust92 deleted the r/secretsmanager_secret_policy branch April 15, 2021 10:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad left review comments

@bill-rich bill-rich bill-rich approved these changes

Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/secretsmanager Issues and PRs that pertain to the secretsmanager 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New resource aws_secretsmanager_resource_policy
5 participants
@DrFaust92 @haidaraM @dblooman @bflad @bill-rich