Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Add policy statements and redrive policy support to sqs_queue. #1474

Closed
wants to merge 3 commits into from
Closed

Add policy statements and redrive policy support to sqs_queue. #1474

wants to merge 3 commits into from

Conversation

sheepkiller
Copy link

No description provided.

@gregdek
Copy link
Contributor

gregdek commented Jan 11, 2016

Thanks @sheepkiller. @loia please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

@sheepkiller
Copy link
Author

@loia
Copy link
Contributor

loia commented Jan 23, 2016

Thanks @sheepkiller.

Regarding the policy statements, would you consider just having a single policy parameter which takes a JSON string? e.g.

policy: "{{ lookup('file','my_sqs_policy.json') }}"

The benefits that I see are:

  • Consistency with the other AWS modules (iam_policy, s3_bucket and sns_topic) which do it this way. iam_policy also supports reading from a file which I don't think is really necessary when you can use lookup.
  • Simplifies the code greatly as you don't need to modify the policy (SID, resource) or handle 'replace vs append'.
  • Easier for admins to manage their IAM policies as separate files across all AWS resources. They don't have to translate from JSON to YAML and can also use jinja templates to reuse policies.

@sheepkiller
Copy link
Author

Hi @loia,

thanks for taking time to review my patch. No problem, I'll update my patch to support policy as json file.

@sivel
Copy link
Member

sivel commented Feb 23, 2016

============================================================================
./cloud/amazon/sqs_queue.py
============================================================================
ERROR: version_added for new option (policy_statements) should be 2.1. Currently 0.0
ERROR: version_added for new option (redrive_policy) should be 2.1. Currently 0.0
ERROR: version_added for new option (statements_strategy) should be 2.1. Currently 0.0

@gregdek
Copy link
Contributor

gregdek commented Mar 24, 2016

Thanks @sheepkiller for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

@gregdek
Copy link
Contributor

gregdek commented Apr 9, 2016

@sheepkiller A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Copy link
Contributor

gregdek commented Apr 24, 2016

@sheepkiller Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Copy link
Contributor

gregdek commented May 10, 2016

@sheepkiller A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Copy link
Contributor

gregdek commented May 25, 2016

@sheepkiller Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@sheepkiller
Copy link
Author

superseded by #1716

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants