-
Notifications
You must be signed in to change notification settings - Fork 403
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
connection/aws_ssm - create S3clientmanager class and move related methods #2255
Open
mandar242
wants to merge
31
commits into
ansible-collections:main
Choose a base branch
from
mandar242:s3clientmanager_class
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
09da93e
create class S3ClientManager, move _get_bucket_endpoint's functionali…
mandar242 f2880a0
move _get_boto_client's functionality to S3ClientManager class
mandar242 44c18de
move _get_url's functionality to S3ClientManager class
mandar242 4e335d8
move _generate_encryption_settings's functionality to S3ClientManager…
mandar242 fb77ca1
add type hints
mandar242 af0ca82
minor fix
mandar242 3b5fb25
reorder client initialization to ensure s3_manager is available befor…
mandar242 6500337
minor fix
mandar242 3e19397
move s3 client initialization to S3ClientManager class
mandar242 d7acf92
copy s3 client from S3ClientManager class to Connection class
mandar242 eddbc1f
adjust unit tests as s3 client is initialized in S3ClientManager class
mandar242 058619b
add changlog fragment
mandar242 8022a6a
black and isort linter fixes
mandar242 f4f640a
isort linter fix
mandar242 029846d
add unit test for s3clientmanager.initialize_s3_client
mandar242 1a66b02
add unit tests for s3clientmanager.get_url
mandar242 21c4870
add unit tests for s3clientmanager.generate_encryption_settings
mandar242 7b9bfac
add unit tests for s3clientmanager.get_boto_client
mandar242 4354b31
add unit tests for s3clientmanager.get_bucket_endpoint
mandar242 b28c13c
merge conflict rebase cleanup
mandar242 0af6b2e
move S3ClientManager to its own file for modularity
mandar242 31f899f
mock S3ClientManager class in test_generate_commands
mandar242 d82f4f8
change initialize_s3_client to initialize_client
mandar242 80acd5e
minor fix
mandar242 dcd20bb
remove unused client definition
mandar242 99c21b7
remove _get_bucket_endpoint in favor of s3clientmanager.get_bucket_en…
mandar242 7e02ccb
Refactor S3ClientManager to explicitly handle S3 client creation
mandar242 fa9596c
add back original comments from get_bucket_endpoint
mandar242 b6340ef
remove _generate_encryption_settings, _get_url, _initialize_s3_client…
mandar242 c24801a
update tests as _initialize_s3_client does not exist anymore and func…
mandar242 fde7a6c
sanity and linter fixes
mandar242 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
3 changes: 3 additions & 0 deletions
3
changelogs/fragments/2255-aws_ssm-refactor-create-s3clientmanager-class.yml
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
--- | ||
minor_changes: | ||
- aws_ssm - Refactor connection/aws_ssm to add new S3ClientManager class and move relevant methods to the new class (https://github.com/ansible-collections/community.aws/pull/2255). |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if you move this to
plugin_utils
like foramazon.aws
collection?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo for now module_utils seem a reasonable place, as we will be adding more classes like
SSMSessionManager
andFileTransferManager
with other jira stories.But I'm open to either module_utils or creating new dir plugin_utils.
@abikouo @alinabuzachis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, I had to put the amazon.aws pieces into plugin_utils rather than module_utils, because the linter will raise an error if you import from something that won't be sent to a remote host.
Personally I would recommend moving this into plugin_utils.
Over in amazon.aws.plugins.plugin_utils we also have the initial framework for an "AWS" connection plugin (plugin_utils.connection.AWSConnectionBase), which might simplify some of this code further.