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 CLI option to override shell used #118

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

jamietanna
Copy link
Contributor

  • Default to sh for Ensure
  • Add CLI option to override shell used

Closes #117.

As noted in saschagrunert#117, we want to override the shell that's used.

In the case that we want to override the shell, we unfortunately run
into the fact that `Ensure` requires `bash`.

We can start by defaulting to using `sh`, which should be fairly safe as
a drop-in.
@jamietanna jamietanna marked this pull request as ready for review September 25, 2023 19:49
run.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #118 (0204af2) into main (a6f8099) will increase coverage by 0.94%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   71.86%   72.80%   +0.94%     
==========================================
  Files           3        3              
  Lines         231      239       +8     
==========================================
+ Hits          166      174       +8     
  Misses         41       41              
  Partials       24       24              

@saschagrunert
Copy link
Owner

saschagrunert commented Sep 26, 2023

Thank you! The lint CI is not happy unfortunately.

@jamietanna jamietanna force-pushed the feature/shell branch 2 times, most recently from d1092e2 to 3d0d038 Compare October 1, 2023 08:56
@saschagrunert
Copy link
Owner

The linter now complains about the line length 🙃

@jamietanna
Copy link
Contributor Author

jamietanna commented Oct 20, 2023

Sorry yeah just trying to sort - unfortunately my local Go version isn't compatible with golangci-lint hence needing the pushes - sorry about that!

I'm trying to use a Docker container in the meantime!

As noted in saschagrunert#117, we want to override the shell that's used.

To do so, we can continue using the default of `bash`, but allow
overriding it with the `--shell` flag.

We need to disable the gosec finding for the new shell argument:

  G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)

As we're comfortable with the risk, as it runs on the user's machine.

Closes saschagrunert#117.
@saschagrunert saschagrunert merged commit 5fce153 into saschagrunert:main Oct 20, 2023
@jamietanna jamietanna deleted the feature/shell branch October 20, 2023 10:49
@jamietanna
Copy link
Contributor Author

Thank you 🚀

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.

Add ability to override shell used
2 participants