search

runfinch / finch

The Finch CLI is an open source client for container development
https://www.runfinch.com
Apache License 2.0
3.51k stars 90 forks source link

fix: parse management commands with proper arguments #876

Closed Shubhranshu153 closed 6 months ago

Shubhranshu153 commented 6 months ago

Issue #, if available: https://github.com/runfinch/finch/issues/827

Description of changes: Addresses https://github.com/runfinch/finch/issues/827 by adding all environment variables at the location of first occurrence of the environment flag

Testing done: Changes tested locally to ensure that the issue is fixed.

$ ./_output/bin/finch container run --debug -it --rm --env="E=v" busybox env DEBU[0000] Creating limactl command: ARGUMENTS: [shell finch sudo -E nerdctl container run -e E=v -it --rm busybox env], LIMA_HOME: /Users/mharwani/work/runfinch/finch/_output/lima/data PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin E=v TERM=xterm HOME=/root Testing done:

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pendo324 commented 6 months ago

LGTM pending the nits. Can you also update the title and description to be more accurate? It seems like it's just copied from #830. It fixes the same type of issue, but it doesn't do exactly what #830 does

mharwani commented 6 months ago

Thanks for the changes. There can be problems with env passed after the image name. For eg:

finch container run alpine echo -e hello

Might need another approach to handle such cases, but we can do that in a different PR.

pendo324 commented 6 months ago

Thanks for the changes. There can be problems with env passed after the image name. For eg:

finch container run alpine echo -e hello

Might need another approach to handle such cases, but we can do that in a different PR.

That's a good point, we don't want to replace any random -e's that are passed in as a command since 

finch container run alpine echo -e hello

is a valid command already and replacing hello with the value of a hello environment variable would change the meaning of the command.

Maybe we need to special-case any commands that take command line arguments (run and exec) specifically?

Shubhranshu153 commented 6 months ago

need to think of a good solution for it and might need to refactor the code, in general i would want to avoid any fiddling with the command parsing as it creates undefined behavior for the user. But there are edge cases here.

For not would keep that separate from this PR as it doesnt seem to be directly related to the issue.