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

Pass extra args to podman run and ansible run #278

Merged
merged 3 commits into from
Sep 19, 2022
Merged

Pass extra args to podman run and ansible run #278

merged 3 commits into from
Sep 19, 2022

Conversation

ArmaanT
Copy link
Contributor

@ArmaanT ArmaanT commented Aug 9, 2022

This PR adds two new options so that users can pass additional arguments to buildah run and podman run commands.

cc @rafmagns-skepa-dreag

@themkat
Copy link
Contributor

themkat commented Aug 10, 2022

I can definitely see that this might be useful in some cases 🙂 Looks good to me!

One little request that will help other users out: It would be really awesome if you could quickly document these new options in the configuration documentation. 🙂 (this is what is shown on the documentation website).

Co-authored-by: Richard H <rafmagns-skepa-dreag@users.noreply.github.com>
Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

I think this is a great addition for the definition in yaml.

+1 @themkat's comment

also please include at least a single test case for both podman run and buildah run

Comment on lines +181 to +188
self.build_parser.add_argument(
"--extra-buildah-run-args",
help="arguments passed to buildah run command (be careful!)"
)
self.build_parser.add_argument(
"--extra-podman-run-args",
help="arguments passed to podman run command (be careful!)"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid this won't work as:

  1. some translation would have to be done: turn a string into list of strings
  2. the arguments would need to be escaped somehow so ansible-bender CLI won't interpret them actually

Copy link
Collaborator

Choose a reason for hiding this comment

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

please let me know if the CLI interface actually worked for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't actually tried the CLI interface, but I tried to follow the format of how --extra-ansible-args worked, going from a single string to a list of strings using shlex.split(). I'm not sure of the specifics around escaping, but these new flags should behave the same way as existing ones. I'll add some tests, try out the cli, and let you know.

@ArmaanT
Copy link
Contributor Author

ArmaanT commented Aug 30, 2022

@TomasTomecek Sorry for the delay on this. I added a test to ensure the yaml config works correctly as well as one to make sure the extra arguments were getting split correctly.

Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

Finally getting back to this since I returned from my vacation. Thank you for taking the time to finish this!

@themkat are you okay with merging? :)

@themkat
Copy link
Contributor

themkat commented Sep 13, 2022

@TomasTomecek Looks good to me 🙂 Had an extra overview just now. Extra tests and documentation are awesome! 🙂

@ArmaanT
Copy link
Contributor Author

ArmaanT commented Sep 13, 2022

Thanks @TomasTomecek and @themkat! Let me know if you'd like me to squash the commits before merge.

@TomasTomecek TomasTomecek merged commit c1c9e5c into ansible-community:master Sep 19, 2022
@TomasTomecek
Copy link
Collaborator

Looks good, thank you so much @ArmaanT!

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.

3 participants