Add Python and Glue version parameters, add check mode #480

ichekaldin · 2021-03-16T02:34:28Z

SUMMARY

Add parameters for Python version and Glue version.

Available Python and Glue version can be found here:
https://docs.aws.amazon.com/glue/latest/dg/add-job.html

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

aws_glue_job

ADDITIONAL INFORMATION

Example:

community.aws.aws_glue_job:
  - name: my-job
    description: My test job
    command_script_location: my-s3-bucket/script.py
    command_python_version: 3
    glue_version: "2.0"
    role: MyGlueJobRole
    state: present

ansibullbot · 2021-03-16T02:36:03Z

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

ichekaldin · 2021-03-16T03:54:48Z

Looks like the tests fail with the following IAM permissions error:

User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible-collections=community.aws=1927.24 is not authorized to perform: glue:GetJob on resource: arn:aws:glue:us-east-1:966509639900:job/shippable-1927-24

This appears to depend on this PR. What's the best way to move it forward?

tremble · 2021-03-16T05:50:03Z

@ichekaldin,

Looking at mattclay/aws-terminator#95 the best bet would be to open a fresh PR. The 'testing' (JSON) policy shouldn't be needed any more as the 'testing' policies now automatically include the core CI policies.

@jillr usually reviews the aws-terminator PRs on Thursday afternoons (US time), so if you can get it pushed in the next 36 hours or so there's a good chance it can be merged quickly.

ichekaldin · 2021-03-16T13:05:59Z

@tremble Thank you. I submitted mattclay/aws-terminator#137.

tremble

Because this is a pre-existing module I'd recommend not setting a default for either of these parameters and letting AWS set the defaults. This avoids the potential for the 'default' versions to change between Ansible/Collection versions in a way which may break someone's playbooks.

plugins/modules/aws_glue_job.py

ansibullbot · 2021-04-01T23:45:39Z

cc @vijayanandsharma
click here for bot help

jillr

I've tested the IAM policies up through line 175 in the integration test file but I'm running into multiple assertion failures.

jillr · 2021-04-01T23:55:17Z

tests/integration/targets/aws_glue_job/tasks/main.yml

+          - glue_job.changed
+          - glue_job.command.python_version == job_info["Job"]["Command"]["PythonVersion"]
+          - glue_job.command.script_location == job_info["Job"]["Command"]["ScriptLocation"]
+          - glue_job.default_arguments == job_info["Job"]["DefaultArguments"]


This assertion is failing

I suspect this is because camel_dict_to_snake_dict converts DefaultArguments to snake_case.

I added DefaultArguments to the exclusion list for camel_dict_to_snake_dict. I believe this resolves the assertion failure.

jillr · 2021-04-01T23:59:05Z

tests/integration/targets/aws_glue_job/tasks/main.yml

+      assert:
+        that:
+          - glue_job_check.changed
+          - not glue_job_check.description


There is no description key being returned, if this is what you want to test for it would need to be:
- '"description" not in glue_job_check'

@jillr I fixed the test syntax (I think).

jillr · 2021-04-02T00:02:28Z

tests/integration/targets/aws_glue_job/tasks/main.yml

+          - job_info["Job"]["Description"] == job_info_idempotent["Job"]["Description"]
+          - job_info["Job"]["GlueVersion"] == job_info_idempotent["Job"]["GlueVersion"]


Both of these assertions are failing

@jillr I believe this is due to the same reason as assertion in line 90.

ansibullbot · 2021-04-02T01:02:55Z

@ichekaldin this PR contains the following merge commits:

3b54db4

Please rebase your branch to remove these commits.

click here for bot help

ansibullbot · 2021-04-02T01:24:27Z

@ichekaldin this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

ansibullbot · 2021-04-02T01:46:02Z

@ichekaldin this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

ichekaldin · 2021-04-08T19:31:54Z

@jillr Is there anything else I could do to move this forward?

jillr · 2021-04-08T21:38:17Z

@ichekaldin Some of the tests are still failing, I was able to comment out enough to get the policies tested though and will deploy those to our production CI. It's helpful to reviewers if we have passing tests to verify changes with.

index cc61c87..e846c0b 100644
--- a/tests/integration/targets/aws_glue_job/tasks/main.yml
+++ b/tests/integration/targets/aws_glue_job/tasks/main.yml
@@ -89,7 +89,7 @@
           - glue_job.command.script_location == job_info["Job"]["Command"]["ScriptLocation"]
           - glue_job.default_arguments == job_info["Job"]["DefaultArguments"]
           - glue_job.description == job_info["Job"]["Description"]
-          - glue_job.glue_version == job_info["Job"]["GlueVersion"]
+          #- glue_job.glue_version == job_info["Job"]["GlueVersion"]
           - glue_job.role == job_info["Job"]["Role"]
 
     - name: Create Glue job (idempotent) (check mode)
@@ -212,7 +212,7 @@
           - glue_job_update_check.command.script_location == job_info_update_check["Job"]["Command"]["ScriptLocation"]
           - glue_job_update_check.default_arguments == job_info_update_check["Job"]["DefaultArguments"]
           - glue_job_update_check.description == job_info_update_check["Job"]["Description"]
-          - glue_job_update_check.glue_version == job_info_update_check["Job"]["Command"]["GlueVersion"]
+          #- glue_job_update_check.glue_version == job_info_update_check["Job"]["Command"]["GlueVersion"]
           - glue_job_update_check.role == job_info_update_check["Job"]["Role"]
 
     - name: Update Glue job
@@ -251,7 +251,7 @@
           - glue_job_update.command.script_location == job_info_update["Job"]["Command"]["ScriptLocation"]
           - glue_job_update.default_arguments == job_info_update["Job"]["DefaultArguments"]
           - glue_job_update.description == job_info_update["Job"]["Description"]
-          - glue_job_update.glue_version == job_info_update["Job"]["Command"]["GlueVersion"]
+          #- glue_job_update.glue_version == job_info_update["Job"]["Command"]["GlueVersion"]
           - glue_job_update.role == job_info_update["Job"]["Role"]

ichekaldin · 2021-04-25T13:54:16Z

@jillr It looks like the tests are passing now. Can this be approved?

Available Python and Glue version can be found here: https://docs.aws.amazon.com/glue/latest/dg/add-job.html Example: ``` community.aws.aws_glue__jobs: - name: my-job description: My test job command_script_location: my-s3-bucket/script.py command_python_version: 3 glue_version: "2.0" role: MyGlueJobRole state: present ```

This is to avoid inadvertently breaking existing playbooks. Defaults will be set by AWS.

Reference: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ Remove duplicate import.

…haracters

Add tests for DefaultArguments.

Use `get_aws_account_info` method to determine AWS partition and accunt ID instead of reinventing the wheel. Rearrange module attributes alphabetically.

tremble · 2021-10-23T06:51:39Z

recheck

ichekaldin · 2021-11-18T17:03:39Z

@tremble Is there any more work that needs to happen with this PR?

plugins/modules/aws_glue_job.py

out dated review, tests now passing

ansible-zuul

LGTM!

inventory_aws_ec2/test: switch to ec2_instance SUMMARY Migrate the inventory_aws_ec2 tests from ec2 to ec2_instance. Also, properly clean up all the resources during the test ISSUE TYPE Bugfix Pull Request COMPONENT NAME inventory_aws_ec2 Reviewed-by: Mark Chappell <None> Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: None <None>

do not include {} boto filter by default Depends-On: ansible-collections#480 When no filter: specified, do not include an empty dict in the list of boto filters because it effectively matches any host. Closes: ansible-collections#457 Closes: ansible-collections#452 ISSUE TYPE Bugfix Pull Request Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None>

ansibullbot removed the needs_triage label Mar 16, 2021

ichekaldin mentioned this pull request Mar 16, 2021

Add AWS Glue Job permissions mattclay/aws-terminator#137

Merged

tremble requested changes Mar 16, 2021

View reviewed changes

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 1, 2021

jillr previously requested changes Apr 2, 2021

View reviewed changes

ansibullbot added the merge_commit This PR contains at least one merge commit. Please resolve! label Apr 2, 2021

ansibullbot removed 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 Apr 2, 2021

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 Apr 16, 2021

ichekaldin and others added 14 commits October 22, 2021 22:53

Fix test syntax. Remove duplicate documentation key

d3c2902

Remove default values for command_python_version and glue_version

c9f8b40

This is to avoid inadvertently breaking existing playbooks. Defaults will be set by AWS.

Add support for tags

fdc258e

Use retries with jittered backoff

16a295a

Reference: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ Remove duplicate import.

Remove duplicate glue_version argument in module definition

d264c94

Update IAM role name for Glue job to make sure it's shorter than 64 c…

6952d6e

…haracters

Properly detect changes in DefaultArguments parameter

a1be7d8

Add tests for DefaultArguments.

Remove duplicate key in documentation

55c53fb

Update JSON path to fix tests

f88dc4c

Minor edit to fix cleanup

6318759

Add purge_tags parameter

e3a80fd

Use `get_aws_account_info` method to determine AWS partition and accunt ID instead of reinventing the wheel. Rearrange module attributes alphabetically.

Do not set defaults for tags

551a36f

Update version_added

dbf5393

tremble approved these changes Nov 25, 2021

View reviewed changes

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

plugins/modules/aws_glue_job.py Outdated Show resolved Hide resolved

Bump version_added

4081f53

alinabuzachis reviewed Nov 25, 2021

View reviewed changes

plugins/modules/aws_glue_job.py Show resolved Hide resolved

alinabuzachis reviewed Nov 25, 2021

View reviewed changes

plugins/modules/aws_glue_job.py Show resolved Hide resolved

tremble requested a review from alinabuzachis November 25, 2021 14:33

alinabuzachis approved these changes Nov 25, 2021

View reviewed changes

markuman added the gate label Nov 25, 2021

ansible-zuul bot approved these changes Nov 25, 2021

View reviewed changes

ansible-zuul bot merged commit 560169b into ansible-collections:main Nov 25, 2021

tremble mentioned this pull request May 26, 2022

[WIP] Adding tags & purge_tags to aws_glue_job module #57

Closed

ichekaldin deleted the ichekaldin/aws_glue_job_parameters branch July 6, 2022 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Python and Glue version parameters, add check mode #480

Add Python and Glue version parameters, add check mode #480

ichekaldin commented Mar 16, 2021

ansibullbot commented Mar 16, 2021

ichekaldin commented Mar 16, 2021

tremble commented Mar 16, 2021

ichekaldin commented Mar 16, 2021

tremble left a comment

ansibullbot commented Apr 1, 2021

jillr left a comment

jillr Apr 1, 2021

ichekaldin Apr 2, 2021

jillr Apr 1, 2021

ichekaldin Apr 2, 2021

jillr Apr 2, 2021

ichekaldin Apr 2, 2021

ansibullbot commented Apr 2, 2021

ansibullbot commented Apr 2, 2021

ansibullbot commented Apr 2, 2021

ichekaldin commented Apr 8, 2021

jillr commented Apr 8, 2021

ichekaldin commented Apr 25, 2021

tremble commented Oct 23, 2021

ichekaldin commented Nov 18, 2021

ansible-zuul bot left a comment

		- job_info["Job"]["Description"] == job_info_idempotent["Job"]["Description"]
		- job_info["Job"]["GlueVersion"] == job_info_idempotent["Job"]["GlueVersion"]

Add Python and Glue version parameters, add check mode #480

Add Python and Glue version parameters, add check mode #480

Conversation

ichekaldin commented Mar 16, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Mar 16, 2021

ichekaldin commented Mar 16, 2021

tremble commented Mar 16, 2021

ichekaldin commented Mar 16, 2021

tremble left a comment

Choose a reason for hiding this comment

ansibullbot commented Apr 1, 2021

jillr left a comment

Choose a reason for hiding this comment

jillr Apr 1, 2021

Choose a reason for hiding this comment

ichekaldin Apr 2, 2021

Choose a reason for hiding this comment

jillr Apr 1, 2021

Choose a reason for hiding this comment

ichekaldin Apr 2, 2021

Choose a reason for hiding this comment

jillr Apr 2, 2021

Choose a reason for hiding this comment

ichekaldin Apr 2, 2021

Choose a reason for hiding this comment

ansibullbot commented Apr 2, 2021

ansibullbot commented Apr 2, 2021

ansibullbot commented Apr 2, 2021

ichekaldin commented Apr 8, 2021

jillr commented Apr 8, 2021

ichekaldin commented Apr 25, 2021

tremble commented Oct 23, 2021

ichekaldin commented Nov 18, 2021

ansible-zuul bot left a comment

Choose a reason for hiding this comment