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

Add generate_mark() function and corresponding unit test. #2235

Conversation

chynasan
Copy link
Contributor

@chynasan chynasan commented Feb 6, 2025

SUMMARY

I created a function inside of the Connection class to generate a random string to replace existing repetitive code. Since this functionality was only currently used in this particular class and still relies on the MARK_LENGTH variable, I did not pull it out as a separate/common utility.

This function generates a random string of characters based on a globally-defined MARK_LENGTH variable, which seem to be used to bookend SSM CLI input.

This existed in the previous iteration of this code, and I did not change it because I was not able to ascertain why 26 was chosen as the length in the first place, and I did not want to make a prescriptive decision around that. There is an existing unit test that calls "5" as the MARK_LENGTH, so I assume that would have worked as well, but did not want to make breaking changes to the existing unit test. This may be something for a future TODO if the team wants to clarify/standardize a length for this string.

I wrote a unit test for this and confirmed that it passes, though I mainly did that for my own benefit and practice, and don't believe that this function really required any unit testing given what it's doing.

Fixes #ACA-2096

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Function to generate a random string for AWS SSM CLI delimiting.

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.

Hey @chynasan,

Code looks fine, you're hitting a bunch of whitespace related sanity/formatting issues. I'd recommend running tox -m format and tox -m lint.

Copy link
Contributor

@mandar242 mandar242 left a comment

Choose a reason for hiding this comment

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

LGTM!

chynasan and others added 2 commits February 6, 2025 14:30
Fixing whitespace errors as suggested

Co-authored-by: Mandar Kulkarni <[email protected]>
@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Feb 6, 2025
@tremble tremble removed the mergeit Merge the PR (SoftwareFactory) label Feb 6, 2025
Copy link
Contributor

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

https://ansible.softwarefactory-project.io/zuul/buildset/b389f4cd2ec246c6a43a3cd65a4d9468

ansible-galaxy-importer FAILURE in 4m 29s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 24s
✔️ ansible-test-splitter SUCCESS in 3m 59s
integration-community.aws-1 FAILURE in 58m 40s
integration-community.aws-2 FAILURE in 25m 54s
integration-community.aws-3 FAILURE in 22m 34s
integration-community.aws-4 FAILURE in 24m 03s
integration-community.aws-5 FAILURE in 24m 08s
integration-community.aws-6 FAILURE in 24m 39s
integration-community.aws-7 FAILURE in 41m 47s
integration-community.aws-8 FAILURE in 23m 54s
integration-community.aws-9 FAILURE in 24m 11s
✔️ integration-community.aws-10 SUCCESS in 4m 02s
integration-community.aws-11 FAILURE in 24m 50s
Skipped 11 jobs

Copy link
Contributor

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

✔️ ansible-galaxy-importer SUCCESS in 3m 08s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 17s
✔️ ansible-test-splitter SUCCESS in 4m 23s
✔️ integration-community.aws-1 SUCCESS in 23m 22s
✔️ integration-community.aws-2 SUCCESS in 13m 49s
✔️ integration-community.aws-3 SUCCESS in 14m 46s
✔️ integration-community.aws-4 SUCCESS in 13m 45s
✔️ integration-community.aws-5 SUCCESS in 13m 01s
✔️ integration-community.aws-6 SUCCESS in 14m 54s
✔️ integration-community.aws-7 SUCCESS in 14m 58s
✔️ integration-community.aws-8 SUCCESS in 13m 31s
✔️ integration-community.aws-9 SUCCESS in 15m 22s
✔️ integration-community.aws-10 SUCCESS in 4m 21s
✔️ integration-community.aws-11 SUCCESS in 14m 20s
Skipped 11 jobs

Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

Nice work!

@markuman markuman added the mergeit Merge the PR (SoftwareFactory) label Feb 7, 2025
Copy link
Contributor

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/2805668e8e044531bc1e7660007c17c0

✔️ ansible-galaxy-importer SUCCESS in 3m 16s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 25s
✔️ ansible-test-splitter SUCCESS in 4m 15s
✔️ integration-community.aws-1 SUCCESS in 24m 41s
✔️ integration-community.aws-2 SUCCESS in 16m 43s
✔️ integration-community.aws-3 SUCCESS in 15m 11s
✔️ integration-community.aws-4 SUCCESS in 15m 41s
✔️ integration-community.aws-5 SUCCESS in 13m 54s
✔️ integration-community.aws-6 SUCCESS in 16m 26s
✔️ integration-community.aws-7 SUCCESS in 13m 49s
✔️ integration-community.aws-8 SUCCESS in 14m 20s
✔️ integration-community.aws-9 SUCCESS in 15m 14s
✔️ integration-community.aws-10 SUCCESS in 3m 58s
✔️ integration-community.aws-11 SUCCESS in 15m 43s
Skipped 11 jobs

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit bde08f9 into ansible-collections:main Feb 7, 2025
86 of 87 checks passed
Copy link

patchback bot commented Feb 7, 2025

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/bde08f92fb187fe1a789847bd670fca46ad50b3f/pr-2235

Backported as #2239

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 7, 2025
SUMMARY

I created a function inside of the Connection class to generate a random string to replace existing repetitive code. Since this functionality was only currently used in this particular class and still relies on the MARK_LENGTH variable, I did not pull it out as a separate/common utility.
This function generates a random string of characters based on a globally-defined MARK_LENGTH variable, which seem to be used to bookend SSM CLI input.
This existed in the previous iteration of this code, and I did not change it because I was not able to ascertain why 26 was chosen as the length in the first place, and I did not want to make a prescriptive decision around that. There is an existing unit test that calls "5" as the MARK_LENGTH, so I assume that would have worked as well, but did not want to make breaking changes to the existing unit test. This may be something for a future TODO if the team wants to clarify/standardize a length for this string.
I wrote a unit test for this and confirmed that it passes, though I mainly did that for my own benefit and practice, and don't believe that this function really required any unit testing given what it's doing.
Fixes #ACA-2096
ISSUE TYPE

Feature Pull Request

COMPONENT NAME

Function to generate a random string for AWS SSM CLI  delimiting.

Reviewed-by: Mark Chappell
Reviewed-by: Mandar Kulkarni <[email protected]>
Reviewed-by: GomathiselviS <[email protected]>
Reviewed-by: Bianca Henderson <[email protected]>
Reviewed-by: Bikouo Aubin
Reviewed-by: Mike Graves <[email protected]>
(cherry picked from commit bde08f9)
Copy link

github-actions bot commented Feb 7, 2025

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 13, 2025
)

This is a backport of PR #2235 as merged into main (bde08f9).
SUMMARY

I created a function inside of the Connection class to generate a random string to replace existing repetitive code. Since this functionality was only currently used in this particular class and still relies on the MARK_LENGTH variable, I did not pull it out as a separate/common utility.
This function generates a random string of characters based on a globally-defined MARK_LENGTH variable, which seem to be used to bookend SSM CLI input.
This existed in the previous iteration of this code, and I did not change it because I was not able to ascertain why 26 was chosen as the length in the first place, and I did not want to make a prescriptive decision around that. There is an existing unit test that calls "5" as the MARK_LENGTH, so I assume that would have worked as well, but did not want to make breaking changes to the existing unit test. This may be something for a future TODO if the team wants to clarify/standardize a length for this string.
I wrote a unit test for this and confirmed that it passes, though I mainly did that for my own benefit and practice, and don't believe that this function really required any unit testing given what it's doing.
Fixes #ACA-2096
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

Function to generate a random string for AWS SSM CLI  delimiting.

Reviewed-by: Mark Chappell
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 mergeit Merge the PR (SoftwareFactory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants