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

Allow run/exec -e with equals sign in value #860

Merged
merged 3 commits into from
Mar 9, 2024
Merged

Allow run/exec -e with equals sign in value #860

merged 3 commits into from
Mar 9, 2024

Conversation

aripollak
Copy link
Contributor

Fixes: #798

Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Needs unit tests. Right now there's no framework to write tests for root compose_run easily, so it may not make sense to write them just right now, but wait until the framework exists.

@aripollak
Copy link
Contributor Author

Happy to add unit tests where appropriate, but I'm a little confused; is there a place that would make sense to add them now?

@p12tic
Copy link
Collaborator

p12tic commented Mar 8, 2024

On a second look a different approach is possible in this particular case.

compose_run() contains a lot of argument processing that could be moved to a separate function (basically everything here https://github.com/containers/podman-compose/blob/main/podman_compose.py#L2419C39-L2455C42). Then we could add unit tests for that function.

Same with compose_exec().

Fixes: #798
Signed-off-by: Ari Pollak <ajp@aripollak.com>
Signed-off-by: Ari Pollak <ajp@aripollak.com>
Signed-off-by: Ari Pollak <ajp@aripollak.com>
@aripollak
Copy link
Contributor Author

aripollak commented Mar 9, 2024

@p12tic how does it look now? Specfying all the expected args in the passed argparse.Namespaces in the tests seemed a little duplicative, but that felt more appropriate for a unit test than calling into PodmanCompose._parse_args(). I think the interface of compose_run_update_container_from_args() would be more ideal if it wasn't changing the container dict in-place, but I'm not familiar enough with the code to go and change it too much.

@p12tic
Copy link
Collaborator

p12tic commented Mar 9, 2024

@aripollak I agree with the entirety of your comment.

The PR looks good, thanks!

@p12tic p12tic merged commit ee01331 into containers:main Mar 9, 2024
4 checks passed
@aripollak aripollak deleted the fix-env-vars-with-equals branch March 9, 2024 21:26
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.

podman-compose run -e doesn't like values with equals signs
2 participants