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 choice of shell per issue 2974 #15166

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Add choice of shell per issue 2974 #15166

merged 1 commit into from
Aug 22, 2017

Conversation

talarczykco
Copy link

@talarczykco talarczykco commented Jun 7, 2017

Tested on Ubuntu and Windows. Use case is primarily for Windows users who desire Bash functionality. interpreter_flag may be added to any block, else OS-based defaults are used.

Example:

resource "null_resource" "test-local-exec-interpreter" {
  provisioner "local-exec" {
    command = "date /T > outfile-cmd.txt"
  }

  provisioner "local-exec" {
    command = "date > outfile-bash.txt"
    interpreter = "bash"
  }

  provisioner "local-exec" {
    command = "date > outfile-bash-path.txt"
    interpreter = "C:\\Program Files\\Cmder\\vendor\\git-for-windows\\bin\\bash.exe"
  }

  provisioner "local-exec" {
    command = "date > outfile-powershell.txt"
    interpreter = "PowerShell"
    interpreter_flag = "-Command"
  }
}

Code and documentation are included. There didn't appear to be a test fixture to update (open to guidance here.)

@talarczykco
Copy link
Author

talarczykco commented Jun 19, 2017

Not sure why this shows a conflict. Diff shows I only added two blocks of text.

@rismoney
Copy link

Git fetch upstream
Git rebase upstream/master
Resolve conflicts.

@apparentlymart
Copy link
Contributor

Hi @mijit! Thanks for working on this and sorry for the delayed response.

We're currently in a feature freeze period for the 0.10.0 release, so we're currently not merging any non-critical PRs to minimize the changes between 0.10.0-rc1 and the final release. However, this change does look like a nice solution; we'll come back here and work on getting this merged once 0.10.0 is out.

In the mean time, it looks like the confict github has detected is in the docs, and is most likely a result of us having rearranged the documentation directory as part of splitting the website across multiple repositories for v0.10.0. Rebasing your branch against the latest master, as @rismoney suggested, may automatically take care of this if git's rename detection is effective here. If not, we can help you get this updated via some more-manual tweaking.

@talarczykco
Copy link
Author

Thanks for the feedback and followup, @rismoney and @apparentlymart. I did the rebase, re-added the doc file, and pushed, but wasn't expecting hundreds of other commits to appear in my PR. Let me know if I did something wrong, or if there's anything I can do to help. Thanks - and, looking forward to v0.10.0!

@apparentlymart apparentlymart self-requested a review August 16, 2017 21:08
@apparentlymart
Copy link
Contributor

Hi @mijit!

We're getting back to normal merging procedures after the 0.10.0 release process now. Sorry for the delay in looking deeply at this.

The separation of interpreter and interpreter_flag, while corresponding some details of the previous implementation, feels a little weird to me as the user interface... the -c and /C options make sense for unix shells and cmd.exe respectively, but other interpreters require different invocations and so I wonder if we could generalize this to make it feel more intuitive for such interpreters. For example:

  provisioner "local-exec" {
    command = "date > outfile-powershell.txt"
    interpreter = ["PowerShell", "-Command"]
  }

  provisioner "local-exec" {
    command = "open(FOO, '>baz.txt') && print FOO 'hello!'"
    interpreter = ["perl", "-e"]
  }

This way the user can provide as many arguments as needed, and we can just add all of them as prefixes for the final argument specified in command.

I was imagining that an empty or omitted interpreter would cause the current behavior of selecting either a unix shell or cmd.exe depending on platform, whereas if any non-empty interpreter is provided at all it circumvents all of the default behavior and uses literally what the user provided.

What do you think? I know it's been a long time since you opened this, so if you no longer have time for another iteration of this I'm happy to make such changes in your stead, along with dealing with the latest round of conflicts.

@talarczykco
Copy link
Author

Hey @apparentlymart, thanks for getting back to me! Yeah, I like your preference for allowing a list of parameters to be specified all-in-one. I thought about proposing that but didn't want to stray too far from the spirit of what came before.

I'm happy to try to revisit this over the weekend, but I can't guarantee when. If you're raring to go at patching this up, I'd be happy to hand over the reigns -- whatever works. If I don't hear back, I'll poke at it the next chance I get. In any event, thanks for the attention to this detail!

@apparentlymart
Copy link
Contributor

Hi again @mijit!

I'm happy to let you make the changes we discussed here. I'll leave you to it for the moment and look out for updates. If you find that you don't have the time to work on it, that's totally fine! Sorry again that I wasn't able to get you that feedback sooner.

@talarczykco
Copy link
Author

OK, added, tested, and fixed up the commits with a nice rebase. Ready for more feedback and testing!

@apparentlymart apparentlymart merged commit 5d5f822 into hashicorp:master Aug 22, 2017
@apparentlymart
Copy link
Contributor

Looks good @mijit! Thanks again for working on this.

@talarczykco talarczykco deleted the issue-2974 branch August 22, 2017 19:56
@ghost
Copy link

ghost commented Apr 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants