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

aws_glue_connection: Add multiple connection types, add check mode #503

Merged
merged 19 commits into from
Mar 29, 2021
Merged

aws_glue_connection: Add multiple connection types, add check mode #503

merged 19 commits into from
Mar 29, 2021

Conversation

ichekaldin
Copy link
Contributor

SUMMARY

Add multiple connection types to aws_glue_connection, add support for check mode.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

aws_glue_connection

ADDITIONAL INFORMATION

Example:

- community.aws.aws_glue_connection:
    name: My connection
    availability_zone: us-east-1a
    connection_properties:
      JDBC_ENFORCE_SSL: "false"
    connection_type: NETWORK
    description: My test connection
    security_groups:
      - test
    subnet_id: subnet-123abc
    state: present

Examples:

```
- community.aws.aws_glue_connection:
    name: My connection
    availability_zone: us-east-1a
    connection_properties:
      JDBC_ENFORCE_SSL: "false"
    connection_type: NETWORK
    description: My test connection
    security_groups:
      - test
    subnet_id: subnet-123abc
    state: present
```
@ansibullbot
Copy link

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request 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 plugins plugin (any type) tests tests labels Mar 26, 2021
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 submit this PR.

Rather than replacing the existing tasks in the integration tests it would be better if the new tasks were added to the tests. This helps to ensure that old playbooks should continue working.

I'd recommend using the jittered backoff rather than a plain backoff: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/

Additionally, we have some magic in AnsibleAWSModule.client() which decorates the client calls. This means you can retry just the API call by adding aws_retry=True to the call rather than needing to retry all of the code around it. See for example https://github.com/ansible-collections/community.aws/pull/421/files

    retry_decorator = AWSRetry.jittered_backoff(retries=10)
    connection = module.client('ec2', retry_decorator=retry_decorator)
...
    response = connection.describe_egress_only_internet_gateways(aws_retry=True)

It's not as important for this module because it's mostly making one call per function, but if (for example) you made the describe call directly inside the create_or_update_glue_connection function, and the describe call failed due to limits, it would be possible for the change to be made during the first run, the describe fails, and then when it re-runs the whole function there's nothing to change, so it'll result in changed=False when a change has happened.

plugins/modules/aws_glue_connection.py Outdated Show resolved Hide resolved
plugins/modules/aws_glue_connection.py Show resolved Hide resolved
@tremble tremble changed the title Add multiple connection types, add check mode aws_glue_connection: Add multiple connection types, add check mode Mar 29, 2021
@tremble
Copy link
Contributor

tremble commented Mar 29, 2021

Hi @ichekaldin,

Thanks for the updates. I'm still a little concerned that the original tests have been completely overwritten. One of the reasons for integration tests is to ensure that playbooks which used to work continue to work. One option that some people use is splitting the tests into multiple task-files and then "including" them. This means you can just add a new set of tasks to test a new feature.

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.

LGTM. Once the tests pass I'll get this merged.

Thanks for taking the time to raise this PR.

This is mostly to re-trigger the tests.
@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 Mar 29, 2021
@tremble tremble merged commit ed32234 into ansible-collections:main Mar 29, 2021
@ichekaldin ichekaldin deleted the ichekaldin/aws_glue_connection_types_check_mode branch April 20, 2021 14:50
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…nsible-collections#503)

* Add multiple connection types and support for check mode

Examples:

```
- community.aws.aws_glue_connection:
    name: My connection
    availability_zone: us-east-1a
    connection_properties:
      JDBC_ENFORCE_SSL: "false"
    connection_type: NETWORK
    description: My test connection
    security_groups:
      - test
    subnet_id: subnet-123abc
    state: present
```
* Add retries.

* Add description of how to create a Glue network connection

Co-authored-by: Mark Chappell <[email protected]>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…nsible-collections#503)

* Add multiple connection types and support for check mode

Examples:

```
- community.aws.aws_glue_connection:
    name: My connection
    availability_zone: us-east-1a
    connection_properties:
      JDBC_ENFORCE_SSL: "false"
    connection_type: NETWORK
    description: My test connection
    security_groups:
      - test
    subnet_id: subnet-123abc
    state: present
```
* Add retries.

* Add description of how to create a Glue network connection

Co-authored-by: Mark Chappell <[email protected]>
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
…nsible-collections#503)

* Add multiple connection types and support for check mode

Examples:

```
- community.aws.aws_glue_connection:
    name: My connection
    availability_zone: us-east-1a
    connection_properties:
      JDBC_ENFORCE_SSL: "false"
    connection_type: NETWORK
    description: My test connection
    security_groups:
      - test
    subnet_id: subnet-123abc
    state: present
```
* Add retries.

* Add description of how to create a Glue network connection

Co-authored-by: Mark Chappell <[email protected]>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jul 6, 2022
…case (#518)

glue_connection - Avoid converting connection_parameter keys to lowercase

SUMMARY

This is a follow to my own PR #503.
This is a cosmetic change that prevents converting keys in connection_parameters dict to lowercase.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

aws_glue_connection
ADDITIONAL INFORMATION



As an example, this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      JDBC_ENFORCE_SSL: "false"
    ...

is a valid value, while this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      jdbc_enforce_ssl: "false"
    ...

is not.
This PR simply aligns the module output to the expected input.

Reviewed-by: Mark Chappell <None>
patchback bot pushed a commit that referenced this pull request Jul 6, 2022
…case (#518)

glue_connection - Avoid converting connection_parameter keys to lowercase

SUMMARY

This is a follow to my own PR #503.
This is a cosmetic change that prevents converting keys in connection_parameters dict to lowercase.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

aws_glue_connection
ADDITIONAL INFORMATION

As an example, this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      JDBC_ENFORCE_SSL: "false"
    ...

is a valid value, while this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      jdbc_enforce_ssl: "false"
    ...

is not.
This PR simply aligns the module output to the expected input.

Reviewed-by: Mark Chappell <None>
(cherry picked from commit b03919d)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jul 6, 2022
…case (#518) (#1321)

[PR #518/b03919db backport][stable-4] glue_connection - Avoid converting connection_parameter keys to lowercase

This is a backport of PR #518 as merged into main (b03919d).
SUMMARY

This is a follow to my own PR #503.
This is a cosmetic change that prevents converting keys in connection_parameters dict to lowercase.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

aws_glue_connection
ADDITIONAL INFORMATION



As an example, this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      JDBC_ENFORCE_SSL: "false"
    ...

is a valid value, while this:
- community.aws.glue_connection:
    name: test-connection
    connection_parameters:
      jdbc_enforce_ssl: "false"
    ...

is not.
This PR simply aligns the module output to the expected input.

Reviewed-by: Mark Chappell <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants