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 multiline output in Dynamic vars #63

Closed
smyrman opened this issue Sep 2, 2017 · 8 comments
Closed

Allow multiline output in Dynamic vars #63

smyrman opened this issue Sep 2, 2017 · 8 comments
Labels
type: feature A new feature or functionality.

Comments

@smyrman
Copy link
Contributor

smyrman commented Sep 2, 2017

There is only one way of setting a multiline variable in task from shell output that I am aware of, and that is using the deprecated set.

Using the sh: program syntax will (deliberately) give an error. I don't know what the reasoning for that is, but there are many good use cases for allowing multiline variables to be set from a shell command, such as fetching private keys from a keychain. In this case you would also prefer if you didn't need to store the private key in a temp file on disk.

@andreynering
Copy link
Member

What exactly do you expect? Do you have an example Taskfile and expected output?

Task disallows that because it breaks sh commands:

do-something -o output {{.INPUT}} # multiline

do-something -o output file1
file2
file3

I just tried and Makefiles replaces line breaks with " ":

FOO := $(shell echo "foo\nbar")

echo:
	# echoes "foo bar"
	@echo $(FOO)
.PHONY: echo

My proposal would be convert multiline vars to []string, so one could either join or loop.

build:
  cmds:
    - minify -o bundle.js {{join .JS_FILES " "}}

or

build:
  cmds:
    - |
      {{range $file := .JS_FILES}}
        minify -o "dist/{{$file}}" "src/{{$file}}"
      {{end}}

@andreynering andreynering added the type: feature A new feature or functionality. label Sep 2, 2017
@smyrman
Copy link
Contributor Author

smyrman commented Sep 2, 2017

If you quote it, it doesn't break the shell cmd.

default:
  vars:
    RSA_KEY:
      sh: pass mysecret  # where pass is a Unix keychain tool, and the stored secret is multi-line.
  cmds:
     - ./myprogram -private-key='{{.RSA_KEY}}'

Today we do the same using set, but the way set works by setting this env globally for every other task with a potential of data-races is not great. Which I assume is part of the reason for it being deprecated?

I don't like the idea of making it a string slice. That's not what you expect to get as a user.

If you want to avoid users making mistakes, it could be a deliberate flag:

default:
  vars:
    RSA_KEY:
      sh: pass mysecret
      multiline: true  # skip multi-line check and don't trim trailing new-lines.

It's probably not super intuitive for new users as you would only need to use this flag if you set a var from sh, not when using another (already multiline) variable, or explicetly declare it multiline in a var via yaml. However it's probably good enough, especially if you get a hint about the flag on errors. It certainly would be for us.

@andreynering
Copy link
Member

andreynering commented Sep 2, 2017

@smyrman I agree it's a bit unintuitive to the user, but we could have a default String() function that just joins it with a space (or maybe a line break). See https://play.golang.org/p/0vjZypJwRe

So most users would not need to bother about it, but could also join with a different value if they want. What do you think about it?

And yes, I want to deprecated set ASAP. I think other than multiline, everything is already possible via task parameters. (Am I missing something?).

@smyrman
Copy link
Contributor Author

smyrman commented Sep 2, 2017

a default String()

That might just work:-)

@smyrman
Copy link
Contributor Author

smyrman commented Sep 2, 2017

Do you want me to do a PR, or will you?

I would like to at least test it to see how it works for our use-case.

@andreynering
Copy link
Member

andreynering commented Sep 2, 2017

It would be nice to have a PR for it if you have time. 👍

If you don't, I'll do with time.

@smyrman
Copy link
Contributor Author

smyrman commented Sep 3, 2017

As a heads-up, when it came to implementation, it still seams better to handle this at command parse time. Replacing the Var.Static attribute with a []string is a bigger and also a breaking change. E.g. it would break both set and cases where the end-users define multiline values in task via yaml:

default:
  vars: 
    MLINE: |
      foo
      bar
      baz
  cmds:
    - echo "{{.MLINE}}"

Instead I am doing the following for the first PR at least (README extract):

Dynamic variables

(...)

By default, the result of the shell command is concatenated to one line, and all
trailing and leading white-space are trimmed. This makes the variable somewhat
safer to use in shell commands, and should work well in most use cases.

For advanced use cases, you may set the multiline and/or notrim flags. By
doing this, you must ensure quoting is used in a safe manner when you use the
variable to not break the shell command into two or more commands. The same is
true for multi-line values supplied via yaml.

build:
  cmds:
    - go build -ldflags='-X main.History="{{.GIT_HISTORY | replace "'" "\'"| replace """ "\""}}"' main.go
  vars:
    GIT_COMMIT:
      sh: git log -n 5
      multiline: true
      notrim: true

END README

The code for this is clean enough, I will push it in a while...

@smyrman
Copy link
Contributor Author

smyrman commented Sep 3, 2017

btw, the other use-cases you mentioned @andreynering, could be handled by template functions:

build:
  cmds:
    - |
      {{range $file := splitList "\n" .JS_FILES }}
        minify -o "dist/{{$file}}" "src/{{$file}}"
      {{end}}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature or functionality.
Projects
None yet
Development

No branches or pull requests

2 participants