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 support for sns_topic_policy #206

Merged
merged 1 commit into from
Feb 5, 2021
Merged

add support for sns_topic_policy #206

merged 1 commit into from
Feb 5, 2021

Conversation

moadibfr
Copy link
Contributor

@moadibfr moadibfr commented Feb 4, 2021

Q A
πŸ› Bug fix? no
πŸš€ New feature? yes
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues #144
❓ Documentation yes

Description

add support for sns_topic_policy

modify sns_topic support so that we create sns_topic_policy for inline
policy

@moadibfr moadibfr requested a review from a team as a code owner February 4, 2021 15:13
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #206 (7041cf1) into main (e4c03fe) will decrease coverage by 0.02%.
The diff coverage is 68.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   69.83%   69.81%   -0.03%     
==========================================
  Files         206      211       +5     
  Lines        4648     4740      +92     
==========================================
+ Hits         3246     3309      +63     
- Misses       1145     1166      +21     
- Partials      257      265       +8     
Impacted Files Coverage Ξ”
pkg/driftctl.go 0.00% <0.00%> (ΓΈ)
pkg/remote/aws/init.go 0.00% <0.00%> (ΓΈ)
pkg/resource/aws/aws_sns_topic_ext.go 60.00% <20.00%> (-40.00%) ⬇️
pkg/resource/aws/aws_sns_topic_policy_ext.go 60.00% <60.00%> (ΓΈ)
pkg/remote/aws/sns_topic_policy_supplier.go 62.96% <62.96%> (ΓΈ)
pkg/middlewares/aws_sns_topic_policy_expander.go 78.78% <78.78%> (ΓΈ)
.../aws/deserializer/sns_topic_policy_deserializer.go 83.33% <83.33%> (ΓΈ)
pkg/iac/deserializers.go 100.00% <100.00%> (ΓΈ)
pkg/resource/aws/aws_sns_topic_policy.go 100.00% <100.00%> (ΓΈ)
... and 4 more

@eliecharra eliecharra added the kind/enhancement New feature or improvement label Feb 5, 2021
"Id": "arn:aws:sns:us-east-1:526954929923:my-topic-with-policy",
"Policy": "{\"Id\":\"__default_policy_ID\",\"Statement\":[{\"Action\":[\"SNS:Subscribe\",\"SNS:SetTopicAttributes\",\"SNS:RemovePermission\",\"SNS:Receive\",\"SNS:Publish\",\"SNS:ListSubscriptionsByTopic\",\"SNS:GetTopicAttributes\",\"SNS:DeleteTopic\",\"SNS:AddPermission\"],\"Condition\":{\"StringEquals\":{\"AWS:SourceOwner\":[]}},\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Resource\":\"arn:aws:sns:us-east-1:526954929923:my-topic-with-policy\",\"Sid\":\"__default_statement_ID\"}],\"Version\":\"2012-10-17\"}"
}
]
Copy link
Contributor

@wbeuil wbeuil Feb 5, 2021

Choose a reason for hiding this comment

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

Since it's only for the state reader, we can have here and inside the tfstate only the policy. We don't need the parent topic. I would just delete them manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

policies := make([]resource.Resource, 0)

for _, value := range topicList {
policy, err := decodeSNSTopicPolicy(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should shadow the variable here from now on to be consistent with what @elie has done on his last PR.

for _, value := range topicList {
    value := value
    policy, err := decodeSNSTopicPolicy(value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

it does not seem to trigger linter error, I'm wondering if this is required there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I edited it anyway

@moadibfr moadibfr force-pushed the fea/sns_topic_policy branch 3 times, most recently from 8d6f280 to 0f8361d Compare February 5, 2021 13:04
modify sns_topic support so that we create sns_topic_policy for inline
policy
@wbeuil wbeuil self-requested a review February 5, 2021 15:11
@eliecharra eliecharra merged commit c107f89 into main Feb 5, 2021
@eliecharra eliecharra deleted the fea/sns_topic_policy branch February 5, 2021 15:12
@eliecharra eliecharra linked an issue Feb 5, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for aws_sns_topic_policy
3 participants