Refactor Some Command-Related Methods in aws_ssm.py
#2248
Conversation
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
beeankha
force-pushed
the
refactor-exec-transport-command-method
branch
from
b46d9dc to
17a29e9
Compare
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 4m 42s (non-voting) |
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 4m 10s (non-voting) |
beeankha
force-pushed
the
refactor-exec-transport-command-method
branch
3 times, most recently
from
6ad3c3d to
599cb1a
Compare
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 3m 54s (non-voting) |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 50s (non-voting) |
beeankha
changed the title
aws_ssm.pyaws_ssm.py
beeankha
requested review from
gravesm,
hakbailey,
mandar242,
brahmanim,
alinabuzachis,
GomathiselviS,
abikouo and
chynasan
|
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 36s (non-voting) |
beeankha
force-pushed
the
refactor-exec-transport-command-method
branch
from
c862d6d to
843b36b
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 16s (non-voting) |
plugins/connection/aws_ssm.py
Outdated
| @@ -433,6 +436,17 @@ def filter_ansi(line: str, is_windows: bool) -> str: | |||
| return line | |||
|
|
|||
|
|
|||
| @dataclass | |||
There was a problem hiding this comment.
I would have used a typed dict instead of a dataclass, but it's ok.
There was a problem hiding this comment.
We've seen both starting to appear, is this something we want to be opinionated about?
There was a problem hiding this comment.
I would have used a typed dict instead of a dataclass, but it's ok.
@alinabuzachis when we discussed some details prior to me making this PR, I totally misunderstood that particular point, so this was a miss on my part; I'm happy to update this to use TypedDict instead. I can update CommandResult to the following:
from typing import TypedDict
class CommandResult(TypedDict):
"""
A custom dictionary containing the executed command results.
"""
returncode: int
stdout_combined: str
stderr_combined: str
|
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 54s (non-voting) |
| if is_windows: | ||
| assert "command" in test_command_generation[0][0] | ||
| assert "method" in test_command_generation[0][1] | ||
| assert "headers" in test_command_generation[0][1] | ||
| assert "Invoke-WebRequest" in test_command_generation[0][1]["command"] | ||
| assert test_command_generation[0][1]["method"] == "put" | ||
| # Two command dictionaries are generated for Windows | ||
| assert len(test_command_generation[0]) == 2 | ||
| else: | ||
| assert "method" in test_command_generation[0][2] | ||
| assert "headers" in test_command_generation[0][2] | ||
| assert "curl --request PUT -H" in test_command_generation[0][2]["command"] | ||
| assert test_command_generation[0][2]["method"] == "put" | ||
| # Three command dictionaries are generated on non-Windows systems | ||
| assert len(test_command_generation[0]) == 3 |
There was a problem hiding this comment.
Not a blocking suggestion, but it seems like we ought to be able to do a more complete test against the contents of test_command_generation. Since you're mocking generate_presigned_url() I think the return value for _generate_commands() shouldn't vary.
There was a problem hiding this comment.
I might be misunderstanding, but the command dictionaries that get generated contain different commands depending on whether the instances are Windows vs non-Windows, so I wanted to make sure that each variation of the commands were output as expected. The details like the URL and S3 path are the same; I do see some assertions I could pull out of the if/else block, though, so I'll update the test to reflect that.
There was a problem hiding this comment.
Yeah, sorry, let me clarify. The tests currently test the structure of what's returned from _generate_command() and partially test the content. What I'm suggesting is that I think we should be able to test the full command that gets returned, rather than just testing that some portion of it contains a specific string. I would argue that if something were to change the command, it should break the test.
If I'm reading the code wrong and it's going to be difficult to test the full command, I think it's fine as is.
|
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 42s (non-voting) |
…aries contained in a tuple
…ries containing commands; add type hinting and docstring
…ate_commands() and _exec_transport_commands(); add type hinting and docstring
…on-Windows systems
beeankha
force-pushed
the
refactor-exec-transport-command-method
branch
from
183e819 to
b4643a0
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 17s (non-voting) |
|
Build succeeded (gate pipeline). ❌ ansible-galaxy-importer FAILURE in 4m 34s (non-voting) |
softwarefactory-project-zuul
bot
merged commit 3111215
into
ansible-collections:main
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #2261 🤖 @patchback |
SUMMARY This work is part of the currently ongoing AWS SSM Connection Refactoring & Plugin Promotion effort and related to ansible-collections/amazon.aws#2394. Fixes ACA-2095 ISSUE TYPE Feature Pull Request TASK LIST The _exec_transport_commands method is refactored to accept a single structured object (a tuple containing lists of command dictionaries) that contains the necessary command data. Create a custom dataclass to hold command output. Create a custom dataclass TypedDict to hold command results. The _generate_commands function returns a list of typed dictionaries with clear metadata (command strings, method, headers). _exec_transport_commands accepts a list of typed dictionaries as an argument and returns the custom CommandResults object. Python type hints are added to the method signature and the structured object to improve readability, facilitate static analysis, and clearly define the expected structure and data types. Docstrings should be added to any refactored methods and the structured object (if custom), clarifying the purpose, structure, inputs, outputs, and any edge cases or special handling (if applicable). Unit tests have been created to ensure that the refactored _generate_commands method outputs the expected structured command object. Add a changelog file. Reviewed-by: Bikouo Aubin Reviewed-by: Rahmanim Benny <[email protected]> Reviewed-by: Alina Buzachis Reviewed-by: GomathiselviS <[email protected]> Reviewed-by: Mark Chappell Reviewed-by: Bianca Henderson <[email protected]> Reviewed-by: Mike Graves <[email protected]> (cherry picked from commit 3111215)
SUMMARY
This work is part of the currently ongoing AWS SSM Connection Refactoring & Plugin Promotion effort and related to ansible-collections/amazon.aws#2394.
Fixes ACA-2095
ISSUE TYPE
TASK LIST
_exec_transport_commandsmethod is refactored to accept a single structured object (a tuple containing lists of command dictionaries) that contains the necessary command data.Create a custom dataclass to hold command output.custom dataclassTypedDictto hold command results._generate_commandsfunction returns a list of typed dictionaries with clear metadata (command strings, method, headers)._exec_transport_commandsaccepts a list of typed dictionaries as an argument and returns the customCommandResultsobject._generate_commandsmethod outputs the expected structured command object.