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

[WIP] dynamodb_table: boto3, AnsibleAwsModule, billing_mode, point_in_time_recovery and more #65

Closed
wants to merge 8 commits into from
Closed

[WIP] dynamodb_table: boto3, AnsibleAwsModule, billing_mode, point_in_time_recovery and more #65

wants to merge 8 commits into from

Conversation

marknet15
Copy link
Contributor

@marknet15 marknet15 commented May 5, 2020 •
edited
Loading

SUMMARY

This is a fresh PR to transfer over the original PR from the ansible repo to the new community.aws repo as requested to meet new requirements.

Original PR's

ansible/ansible#45402
ansible/ansible#68320
anryko/ansible#1

I'm providing the fixes listed below for the changes proposed in ansible/ansible#68320 that are now stale that is in turn based on the below:

Fix AnsibleAwsModule migration stale PR ansible/ansible#45402. Add billing_mode and point_in_time_recovery. > Small bug fixes. Integration tests fixes.

Fixes:

  • Fix issue with ProvisionedThroughput params being set incorrectly on GSI's when billing is PAY_PER_REQUEST.
  • Fix incorrect key_type_mapping.
  • Fix defining of serialized_index_attribute_definitions for both the hash and the range key on a GSI. (range key definition was being overwritten).
  • Fix defining of SSE spec.

Fixes #45402
Fixes #56923

ISSUE TYPE

  • Bugfix Pull Request
  • Feature Pull Request

COMPONENT NAME

  • dynamodb_table

ADDITIONAL INFORMATION

- name: enable on-demand billing and backups
  dynamodb_table:
  name: table_name
  hash_key_name: myhashkey
  range_key_name: myrangekey
  billing_mode: PAY_PER_REQUEST
  point_in_time_recovery: yes
  indexes:
      - name: myindex
          type: global_all
          hash_key_name: somekey
          range_key_name: myrangekey

marknet15 and others added 7 commits May 5, 2020 17:10
@marknet15 @anryko @sramakr
Transfer PR from ansible repo
9860b8c 
https: //github.com/ansible/ansible/pull/45402
https: //github.com/ansible/ansible/pull/68320
https: //github.com/anryko/ansible/pull/1
Co-Authored-By: Andrej Svenke <a.shvenke@gmail.com>
Co-Authored-By: sramakr <sramakr@users.noreply.github.com>
@marknet15
Fix imports and tests
ab7325b
@marknet15
Test fixes
d336c2a
@marknet15
Missed extra arg
a06d870
@marknet15
Fix test and typos
52fb4eb
@marknet15
Fix incorrect update
a030c6e
@marknet15
Fix import & test
a914f68
@marknet15 marknet15 mentioned this pull request May 11, 2020
Closed
jillr
jillr reviewed May 14, 2020
View reviewed changes
tests/integration/targets/dynamodb_table/tasks/main.yml
write_capacity: 10
register: result
until:
- result | json_query('indexes[?name==`NamedIndexV2`].read_capacity') == [10]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The json_query filter has moved to a collection, so these will need to be community.general.json_query.

tests/integration/targets/dynamodb_table/tasks/main.yml
dynamodb_table:
name: "{{ table_name }}"
hash_key_name: myhashkey
indexes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

AWS permits only one index to be created in a single action, this task is throwing an unhandled ClientError. The code should reflect and enforce the service limits.
botocore.errorfactory.LimitExceededException: An error occurred (LimitExceededException) when calling the UpdateTable operation: Subscriber limit exceeded: Only 1 online index can be created or deleted simultaneously per table
https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Limits.html#limits-secondary-indexes

I haven't done a proper review of the code, but there doesn't seem to be much exception handling. We generally like to catch ClientError and BotoCoreError.

@jillr jillr changed the base branch from master to main July 2, 2020 19:48
@ansibullbot
Copy link

cc @jillr @loia @s-hertel @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage new_contributor Help guide this first time contributor stale_ci CI is older than 7 days, rerun before merging tests tests labels Aug 19, 2020
@Akasurde Akasurde mentioned this pull request Aug 20, 2020
Closed
@ansibullbot ansibullbot added the plugins plugin (any type) label Aug 28, 2020
@tremble tremble linked an issue Oct 12, 2020 that may be closed by this pull request
AWS_URL isn't passed into dynamodb_table #146
Closed
@tremble tremble mentioned this pull request Oct 12, 2020
Closed
@tremble tremble changed the title dynamodb_table: boto3, AnsibleAwsModule, billing_mode, point_in_time_recovery and more [WIP] dynamodb_table: boto3, AnsibleAwsModule, billing_mode, point_in_time_recovery and more Dec 1, 2020
@tremble tremble added the pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 label Dec 1, 2020
@ansibullbot ansibullbot added WIP Work in progress and removed needs_triage new_contributor Help guide this first time contributor labels Jan 13, 2021
@falsedlah
Copy link

Hi guys,
What is the status on this pr? Is there something blocking it? I need this feature and if there is something I can help with to get it move forward, please let me know.

@marknet15
Copy link
Contributor Author

Hey I completely forgot to pick this back up again, I'll try and get it all up-to-date again and address the feedback on the PR

@ansibullbot
Copy link

@marknet15 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 27, 2021
This was referenced Sep 20, 2021
Merged
Merged
@tremble
Copy link
Contributor

tremble commented Sep 20, 2021

I've started working on a simple boto3 migration (#726) while that won't add the extra features it should be easier to review and much simpler to add features to an existing framework.

@marknet15
Copy link
Contributor Author

@tremble Ah I had completely forgotten about this PR, I'll close it then I'll create a fresh PR with the extra functionality after your boto -> boto3 migration

@marknet15 marknet15 closed this Sep 20, 2021
ansible-zuul bot added a commit that referenced this pull request Oct 11, 2021
@ansible-zuul
Merge pull request #726 from tremble/boto3/dynamodb_table
e3a50c4 
dynamodb_table - boto3 migration

SUMMARY
Existing boto3 migration (#65) seems to have been mangled by the migration to collections and has bit-rotted.  Start from scratch and don't add new features.
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
dynamodb_table
ADDITIONAL INFORMATION

Reviewed-by: Mark Woolley <mw@marknet15.com>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <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

Assignees
No one assigned
Labels
affects_2.10 bug This issue/PR relates to a bug has_issue integration tests/integration merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 stale_ci CI is older than 7 days, rerun before merging tests tests WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWS_URL isn't passed into dynamodb_table
5 participants
@marknet15 @ansibullbot @falsedlah @tremble @jillr