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

fix: parse management commands with proper arguments #876

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

Shubhranshu153
Copy link
Contributor

@Shubhranshu153 Shubhranshu153 commented Mar 28, 2024

Issue #, if available: #827

Description of changes:
Addresses #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:

  • I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

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

cmd/finch/nerdctl.go Show resolved Hide resolved
cmd/finch/nerdctl.go Outdated Show resolved Hide resolved
@pendo324
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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.

@Shubhranshu153 Shubhranshu153 merged commit e2f42fe into runfinch:main Mar 28, 2024
22 checks passed
Shubhranshu153 pushed a commit that referenced this pull request Mar 29, 2024
🤖 I have created a release *beep* *boop*
---


## [1.1.3](v1.1.2...v1.1.3)
(2024-03-28)


### Build System or External Dependencies

* **deps:** Bump github.com/aws/aws-sdk-go-v2 from 1.25.0 to 1.25.2
([#831](#831))
([9eb8097](9eb8097))
* **deps:** Bump github.com/aws/aws-sdk-go-v2 from 1.25.2 to 1.25.3
([#856](#856))
([e9314f0](e9314f0))
* **deps:** Bump github.com/aws/aws-sdk-go-v2 from 1.25.3 to 1.26.0
([#864](#864))
([9223219](9223219))
* **deps:** Bump github.com/docker/cli from 25.0.3+incompatible to
25.0.4+incompatible
([#857](#857))
([838fc07](838fc07))
* **deps:** Bump github.com/docker/cli from 25.0.4+incompatible to
26.0.0+incompatible
([#867](#867))
([4167d3d](4167d3d))
* **deps:** Bump github.com/docker/docker from 25.0.3+incompatible to
25.0.4+incompatible
([#845](#845))
([769ae32](769ae32))
* **deps:** Bump github.com/docker/docker from 25.0.4+incompatible to
26.0.0+incompatible
([#866](#866))
([7fa6e72](7fa6e72))
* **deps:** Bump github.com/lima-vm/lima from 0.20.1 to 0.20.2
([#855](#855))
([99d4c3c](99d4c3c))
* **deps:** Bump github.com/lima-vm/lima from 0.20.2 to 0.21.0
([#862](#862))
([120ffcc](120ffcc))
* **deps:** Bump github.com/onsi/ginkgo/v2 from 2.15.0 to 2.16.0
([#842](#842))
([8e04421](8e04421))
* **deps:** Bump github.com/onsi/gomega from 1.31.1 to 1.32.0
([#860](#860))
([b2c9449](b2c9449))
* **deps:** Bump github.com/runfinch/common-tests from 0.7.13 to 0.7.14
([#834](#834))
([2287575](2287575))
* **deps:** Bump github.com/shirou/gopsutil/v3 from 3.24.1 to 3.24.2
([#838](#838))
([d2612aa](d2612aa))
* **deps:** Bump github.com/stretchr/testify from 1.8.4 to 1.9.0
([#839](#839))
([9f0953d](9f0953d))
* **deps:** Bump golang.org/x/crypto from 0.19.0 to 0.20.0
([#833](#833))
([1ed1474](1ed1474))
* **deps:** Bump golang.org/x/crypto from 0.20.0 to 0.21.0
([#843](#843))
([b7ef6f2](b7ef6f2))
* **deps:** Bump golang.org/x/tools from 0.18.0 to 0.19.0
([#844](#844))
([f8883b2](f8883b2))
* **deps:** Bump google.golang.org/protobuf from 1.32.0 to 1.33.0
([#858](#858))
([14532d5](14532d5))
* **deps:** Bump k8s.io/apimachinery from 0.29.2 to 0.29.3
([#863](#863))
([c8a4262](c8a4262))
* **deps:** Bump submodules and dependencies
([#825](#825))
([8828c56](8828c56))


### Bug Fixes

* parse management commands with proper arguments
([#876](#876))
([e2f42fe](e2f42fe))
* Reset disks and force remove vm after suite execution
([#846](#846))
([c2363b1](c2363b1))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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