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

add KmsMasterKeyId as attribute option for boto3 call #762

Merged
merged 8 commits into from Oct 27, 2021
Merged

add KmsMasterKeyId as attribute option for boto3 call #762

merged 8 commits into from Oct 27, 2021

Conversation

falcon78921
Copy link
Contributor

SUMMARY

When creating a SQS queue, passing a value for KmsMasterKeyId does not enable SSE. This PR fixes how attributes, like KmsMasterKeyId, are passed into the boto3 invocation.

Fixes: #698

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

sqs_queue.py

@falcon78921 falcon78921 changed the title [WIP] pass KmsMasterKeyId into boto3 call [WIP] pass KmsMasterKeyId into boto3 call Oct 15, 2021
@ansibullbot
Copy link

cc @jillr @loia @markuman @nand0p @s-hertel @sbj-ss @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Oct 15, 2021
@falcon78921 falcon78921 changed the title [WIP] pass KmsMasterKeyId into boto3 call [WIP] add KmsMasterKeyId as attribute option for boto3 call Oct 15, 2021
@falcon78921 falcon78921 changed the title [WIP] add KmsMasterKeyId as attribute option for boto3 call add KmsMasterKeyId as attribute option for boto3 call Oct 15, 2021
@ansibullbot ansibullbot added community_review integration tests/integration tests tests and removed WIP Work in progress labels Oct 15, 2021
jillr
jillr reviewed Oct 19, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Thanks @falcon78921, couple of minor comments

tests/integration/targets/sqs_queue/tasks/main.yml Show resolved Hide resolved
tests/integration/targets/sqs_queue/tasks/main.yml Show resolved Hide resolved
changelogs/fragments/762-fix-kmsmasterkeyid-attr.yaml Outdated Show resolved Hide resolved
@ansibullbot
Copy link

@falcon78921 This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Oct 26, 2021
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 26, 2021
tremble
tremble requested changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to open this PR. A couple of minor niggles that I'll commit, if the tests still pass I think we can get this merged.

plugins/modules/sqs_queue.py Outdated Show resolved Hide resolved
tests/integration/targets/sqs_queue/tasks/main.yml Outdated
- name: Test queue features
block:
- name: Test create queue with attributes
sqs_queue:
name: "{{ resource_prefix }}{{ 1000 | random }}"
default_visibility_timeout: 900
delivery_delay: 900
# Test SSE encryption
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Test SSE encryption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @tremble. I just noticed that the comment should be Test SSE, but you don't want any comments around test tasks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally the test description like this would be in the name block up top. having it listed here doesn't bring much value, we already know what that parameter is supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already know what that parameter is supposed to do.

Although kms_master_key_id involves encryption, I didn't know specifying kms_master_key_id automatically turned it on. I was thinking there was a separate encryption key that corresponded with a boolean value. The boto3 docs don't really jump out and say it either (https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/sqs.html#SQS.Client.create_queue), unless I missed something. If you don't want a mention in the tests, do you think adding something in the module docs would help clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

As someone using this feature, if you think the docs need a tweak to help folks understand what's going on then most certainly update them.

Only the docs at the top of the Python file need to be updated. The RST is auto-generated from them.

tests/integration/targets/sqs_queue/tasks/main.yml Show resolved Hide resolved
@tremble
Copy link
Contributor

tremble commented Oct 27, 2021

Thanks for taking the time to open this PR. A couple of minor niggles that I'll commit, if the tests still pass I think we can get this merged.

Ok, you didn't tick the box to allow maintainers to update the PR so I can't commit them.

@falcon78921
Copy link
Contributor Author

thanks again @tremble 👍

@tremble
Copy link
Contributor

tremble commented Oct 27, 2021

In the interests of getting this PR merged, I'm going to gate this one too. :)

Further tweaks to the documentation to help make it easier to use would most certainly be welcome.

@tremble tremble added the gate label Oct 27, 2021
ansible-zuul[bot]
ansible-zuul bot approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit 1a3ec82 into ansible-collections:main Oct 27, 2021
@falcon78921 falcon78921 deleted the aws-sqs-698 branch October 28, 2021 12:54
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
@alinabuzachis
aws_s3 - ensure overwrite: different (ansible-collections#811)
65f6fa4 
aws_s3 - ensure overwrite: different

SUMMARY

aws_s3 - ensure overwrite: different instead of always in order for the module to be idempotent by default.
Fixes: ansible-collections#762

ISSUE TYPE


Feature Pull Request

COMPONENT NAME

aws_s3

Reviewed-by: Jill R <None>
Reviewed-by: Mark Chappell <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr left review comments

@tremble tremble tremble approved these changes

@ansible-zuul ansible-zuul[bot] ansible-zuul approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug community_review integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

community.aws.sqs_queue encryption not work
4 participants
@falcon78921 @ansibullbot @tremble @jillr