glue_connection - Avoid converting connection_parameter keys to lowercase #518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansibullbot
added
bug
|
Thanks for this patch @ichekaldin. However this would be a breaking change, and would need a deprecation period where we would maintain both formats. I haven't really used Glue myself, but it looks like the AWS docs use the upper-cased values https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/glue.html#Glue.Client.get_connection. I'd like to hear what other Glue users think about this change. Can the lower-cases values be used as the input for another Glue task? |
|
@jillr from your point of view - is it worth working on this PR further? |
alinabuzachis
added a commit
to alinabuzachis/community.aws
that referenced
this pull request
May 25, 2022
Doc fix ec2_vol_info
SUMMARY
attachment_set has always be returned as a list of dictionaries, but the module documented it as a dictionary instead. Let's fix this documentation mismatch.
"attachment_set": [{
"attach_time": "2015-10-23T00:22:29.000Z",
"deleteOnTermination": "false",
"device": "/dev/sdf",
"instance_id": "i-8356263c",
"status": "attached"
}]
Fixes: ansible-collections#515
ISSUE TYPE
Docs Pull Request
COMPONENT NAME
ec2_vol_info
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <None>
|
@ichekaldin apologies for the delay here. In my opinion, this is the correct change for the long run, the question is how to 'deprecate' the old key. We're currently discussing a general cleanup of return values (ansible-collections/amazon.aws#865) To ensure people have a deprecation cycle available to them the consensus seems to be
As such, I'd suggest here is that we add a new return value |
ansibullbot
added
needs_rebase
This is a follow to my own PR #503: #503 This is a cosmetic change that prevents converting keys in connection_parameters dict to lowercase. 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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ansibullbot
added
the
merge_commit
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
This comment was marked as outdated.
This comment was marked as outdated.
This is a follow to my own PR #503: #503 This is a cosmetic change that prevents converting keys in connection_parameters dict to lowercase. 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.
tremble
removed
the
merge_commit
tremble
removed
needs_rebase
|
@ichekaldin I've just rebased this PR rather than "merged" main into the PR. If you merge main into your branch rather than rebasing it can make something of a mess. |
tremble
requested changes
Jul 6, 2022
changelogs/fragments/518-aws_glue_connection-do-not-lowercase-connection-parameters.yml
Outdated
Show resolved
Hide resolved
ansibullbot
added
community_review
and removed
new_module
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@tremble Thank you for all the suggestions and edits! |
tremble
approved these changes
Jul 6, 2022
tremble
changed the title
Backport to stable-4: 💚 backport PR created✅ Backport PR branch: Backported as #1321 🤖 @patchback |
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)
ichekaldin
deleted the
aws_glue_connection_do_not_lowercase_connection_parameters
branch
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>
1 task
softwarefactory-project-zuul bot
pushed a commit
that referenced
this pull request
May 6, 2024
Bump 2024 deprecations from dates to release versions SUMMARY Bump 2024 deprecations from dates to release versions ISSUE TYPE Docs Pull Request COMPONENT NAME plugins/modules/ecs_cluster.py plugins/modules/ecs_service.py plugins/modules/glue_connection.py ADDITIONAL INFORMATION See also: #518 #1640 #1716 Reviewed-by: Alina Buzachis
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
COMPONENT NAME
aws_glue_connection
ADDITIONAL INFORMATION
As an example, this:
is a valid value, while this:
is not.
This PR simply aligns the module output to the expected input.