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

Refactor Some Command-Related Methods in aws_ssm.py #2248

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

beeankha
Copy link

@beeankha beeankha commented Feb 17, 2025

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 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.

@beeankha beeankha added the WIP Work in progress label Feb 17, 2025
@beeankha beeankha self-assigned this Feb 17, 2025
Copy link
Contributor

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.
Warning:
Error merging github.com/ansible-collections/community.aws for 2248,f0da9b82ad297cf0279d614cc1031dcf568760e3

…ries containing commands; add type hinting and docstring
…ate_commands() and _exec_transport_commands(); add type hinting and docstring
@beeankha beeankha force-pushed the refactor-exec-transport-command-method branch from b46d9dc to 17a29e9 Compare February 17, 2025 17:18
Copy link
Contributor

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/c55abb9436424eafb32790c7d6b77d5a

ansible-galaxy-importer FAILURE in 4m 42s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 25s
✔️ ansible-test-splitter SUCCESS in 4m 02s
integration-community.aws-1 FAILURE in 58m 33s
integration-community.aws-2 FAILURE in 25m 09s
integration-community.aws-3 FAILURE in 25m 35s
integration-community.aws-4 FAILURE in 27m 15s
integration-community.aws-5 FAILURE in 26m 27s
integration-community.aws-6 FAILURE in 26m 48s
integration-community.aws-7 FAILURE in 24m 43s
integration-community.aws-8 FAILURE in 25m 47s
integration-community.aws-9 FAILURE in 24m 38s
✔️ integration-community.aws-10 SUCCESS in 6m 37s
integration-community.aws-11 FAILURE in 28m 50s
Skipped 11 jobs

Copy link
Contributor

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/ca7cffedbcb04b7099bd326806e84762

ansible-galaxy-importer FAILURE in 4m 10s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 29s
✔️ ansible-test-splitter SUCCESS in 4m 17s
integration-community.aws-1 TIMED_OUT in 1h 00m 20s
integration-community.aws-2 FAILURE in 30m 54s
integration-community.aws-3 FAILURE in 24m 33s
integration-community.aws-4 FAILURE in 25m 26s
integration-community.aws-5 FAILURE in 25m 04s
integration-community.aws-6 FAILURE in 31m 00s
integration-community.aws-7 FAILURE in 27m 15s
integration-community.aws-8 FAILURE in 23m 38s
integration-community.aws-9 FAILURE in 30m 30s
✔️ integration-community.aws-10 SUCCESS in 6m 20s
integration-community.aws-11 FAILURE in 25m 16s
Skipped 11 jobs

@beeankha beeankha force-pushed the refactor-exec-transport-command-method branch 2 times, most recently from 02cf388 to 6ad3c3d Compare February 25, 2025 21:49
@beeankha beeankha force-pushed the refactor-exec-transport-command-method branch from 6ad3c3d to 599cb1a Compare February 25, 2025 22:09
Copy link
Contributor

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/ce79518076784bdebfdb69270b1914b2

ansible-galaxy-importer FAILURE in 3m 54s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 15s
✔️ ansible-test-splitter SUCCESS in 4m 12s
integration-community.aws-1 FAILURE in 7m 52s
integration-community.aws-2 FAILURE in 29m 52s
integration-community.aws-3 FAILURE in 19m 08s
integration-community.aws-4 FAILURE in 6m 39s
integration-community.aws-5 FAILURE in 25m 23s
integration-community.aws-6 FAILURE in 20m 26s
integration-community.aws-7 FAILURE in 25m 44s
integration-community.aws-8 FAILURE in 24m 22s
integration-community.aws-9 FAILURE in 25m 14s
✔️ integration-community.aws-10 SUCCESS in 4m 40s
integration-community.aws-11 FAILURE in 27m 58s
Skipped 11 jobs

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/fc9bb8d8029d4fc3b58d557f8dc5f3e6

✔️ ansible-galaxy-importer SUCCESS in 5m 50s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 55s
✔️ ansible-test-splitter SUCCESS in 4m 03s
✔️ integration-community.aws-1 SUCCESS in 23m 09s
✔️ integration-community.aws-2 SUCCESS in 13m 37s
✔️ integration-community.aws-3 SUCCESS in 15m 25s
✔️ integration-community.aws-4 SUCCESS in 15m 21s
✔️ integration-community.aws-5 SUCCESS in 25m 13s
✔️ integration-community.aws-6 SUCCESS in 18m 20s
✔️ integration-community.aws-7 SUCCESS in 14m 00s
✔️ integration-community.aws-8 SUCCESS in 13m 58s
✔️ integration-community.aws-9 SUCCESS in 17m 21s
✔️ integration-community.aws-10 SUCCESS in 5m 45s
✔️ integration-community.aws-11 SUCCESS in 13m 34s
Skipped 11 jobs

@beeankha beeankha removed the WIP Work in progress label Feb 26, 2025
@beeankha beeankha changed the title [WIP] Refactor Some Command-Related Methods in aws_ssm.py Refactor Some Command-Related Methods in aws_ssm.py Feb 26, 2025
Copy link
Contributor

@abikouo abikouo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to add a test skipped depending on the platform because the is_windows param is computed from the EC2 instance, not the source computer.
You can combine both tests into a single one.

Comment on lines +307 to +314
@patch("ansible_collections.community.aws.plugins.connection.aws_ssm.Connection.is_windows")
def test_generate_commands_windows(self, mock_is_windows):
"""Testing command generation on Windows systems"""
pc = PlayContext()
new_stdin = StringIO()
conn = connection_loader.get("community.aws.aws_ssm", pc, new_stdin)

mock_is_windows.return_value = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@patch("ansible_collections.community.aws.plugins.connection.aws_ssm.Connection.is_windows")
def test_generate_commands_windows(self, mock_is_windows):
"""Testing command generation on Windows systems"""
pc = PlayContext()
new_stdin = StringIO()
conn = connection_loader.get("community.aws.aws_ssm", pc, new_stdin)
mock_is_windows.return_value = True
@pytest.mark.parametrize("is_windows", [True, False])
@patch("ansible_collections.community.aws.plugins.connection.aws_ssm.Connection.is_windows")
def test_generate_commands_windows(self, mock_is_windows, is_windows):
"""Testing command generation on Windows systems"""
pc = PlayContext()
new_stdin = StringIO()
conn = connection_loader.get("community.aws.aws_ssm", pc, new_stdin)
mock_is_windows.return_value = is_windows

Comment on lines +335 to +340
# Check contents of command dictionaries
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Check contents of command dictionaries
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"
if not is_windows:
# Check contents of command dictionaries
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"
else:
# Check contents of command dictionaries
assert "command" in test_command_generation[0][0]
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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants