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

Templating, command lists and file globs #82

Closed
twhiston opened this issue Dec 18, 2017 · 31 comments · Fixed by #1220
Closed

Templating, command lists and file globs #82

twhiston opened this issue Dec 18, 2017 · 31 comments · Fixed by #1220
Labels
type: feature A new feature or functionality.
Milestone

Comments

@twhiston
Copy link

twhiston commented Dec 18, 2017

Maybe i just am not looking at this from the right angle but I was trying to use the templating to generate a series of commands that can be run based on the input variables. For example lets say i have a list of variables for dockerfile versions/contexts

NS: tomwhiston
REPO: my-project
NAME: my-project
VERSIONS: |
  prod
  prodv3-builder
  prodv3-runtime
  test-base
  test

and the following task

build:
  cmds:
    - |
      {{- range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}}

Currently this task will always succeed, even if, for example, one of the contexts does not exist. So how would one go about implementing something like this, where any command that fails will abort operation, and without having a lot of calls to a task and setting specific vars to pass?
ie not

- task: build
  vars: {NS: "tomwhiston", REPO: "my-project", VERSION: "prod" }

Ideally the outcome I would like is that if I add a new context I simple name it in the vars list and run the commands unaltered.
If this is not possible currently would you be interested in such a feature being added?

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

What happens if you start with set -e?

build:
  cmds:
    - |
      set -e
      {{- range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}}

@twhiston
Copy link
Author

that will only work if you do

build:
  cmds:
    - |
      set -e
      {{ range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}}

note the subtle difference with removing the - from the start of the range. If you don't do this it will just make a broken first command that does not fail, so it's not obvious to catch. So it seems error prone to work this way as subtle problems may be silent. It also causes some issues if you need to change into a folder, because the whole output seen as one command so you are not returned to the root folder after each execution. This makes chaining together some cd ./wherever calls in your command a fail. You can solve this by using pushd and popd instead of cd, but its then not portable as pushd is not available in posix sh (for example).
To me it would be cleaner if it was possible to implement another key maybe template: and then be able to generate via a go template a slice of commands that would be run independently. If you are interested in that idea i would for sure be up for working on it.

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

makes sense

@twhiston
Copy link
Author

cool, i'll try to implement this and make a pr over the next week or so

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

note the subtle difference with removing the - from the start of the range.

Sorry, a bit quick reply, I was actually replying to your this comment 👆. As for making a template: key-word or similar for generating multiple commands it would be great to get @andreynering's opinion first. I think it could be a useful feature, although personally I would be happy with whatever works (including the for loop within a command), even if there would be likely to be some initial debugging issues. Documentation could be added to make it less painful.

In task we get a lot of features through the templating system, that could feel more natural if included in the yaml layout (not only this case, but also e.g. variable defaults); it's hard to know when it is actually more beneficial to (re-)implement them, and have the complexity of (maintaining) two ways to achieve the same goal?

. You can solve this by using pushd and popd instead of cd, (...)

For simple casese, cd - might work, to cd to the previous folder.

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

If you don't do this it will just make a broken first command that does not fail, so it's not obvious to catch.

The simplest thing is probably to not use the whitespace control (-) operator at all, but rather let there be some blank lines in the generated shell script -- did it work btw?

@andreynering
Copy link
Member

andreynering commented Dec 19, 2017

If I understood well, this feature request overlap with #29 and #73. Am I wrong?

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

The linkage against #29 is an interesting one, because that describes a case that is not really implementable through the template syntax - i.e. issue multiple commands with different parameters.

Regarding #73, I haven't really tried to use the watch feature, so I can't comment to much there...

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

Maybe something like this?

default:
  cmds:
    - echo hello
    - range: "my world task"
      split: " "
      cmd: ecoh-task
    - range: "my world echo"  # split: default to white-space,
                              # rangeVar: default to ELEMENT,
                              # rangeIndex: default to INDEX?
      split: " "
      sh: ecoh {{.INDEX}} {{.ELEMENT}}

echo-task:
  cmds:
    - echo {{.INDEX}} {{.ELEMENT}}

@twhiston
Copy link
Author

twhiston commented Dec 19, 2017

I would be happy with whatever works (including the for loop within a command), even if there would be likely to be some initial debugging issues.

What I personally dislike about that is that you have false positives when things are not configured correctly, which can lead you to believe that everything is running correctly if not checking the output carefully. If it failed at any time anything was wrong then i would be totally on board with not adding a new key/feature for this. Its just if I want to use task to orchestrate some testing actions success by default is not something that is suitable for us.

The simplest thing is probably to not use the whitespace control (-) operator at all

I suppose this is true, but its a common part of templating syntax and you cant stop people using it, working in a company and using task i want my output to be as clean as possible for the rest of the team to read, and to remove all chances that a seemingly unrelated change (eating some whitespace) changes the actual execution of the tasks.

If I understood well, this feature request overlap with #29 and #73. Am I wrong?

I guess this is closer to #29 than #73 as i dont really care about watching files, it's more about writing a very generic command that you can pass variables to for execution and which runs each command separately, assessing the exit code.

what i was thinking was something like

template:
   -|
     {{- range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}} 
  -|
   {{- range $i, $version := .VERSIONS | splitLines -}}
         dgoss run ...
   {{end}}

which would produce an output like

- docker build ...
- docker build ...
- dgoss  run ...
- dgoss  run ...

etc.......
the output could be treated like a normal cmd slice and just iterate through it running the commands and checking the exit codes

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

@twhiston,

what i was thinking was something like

template:
   -|
     {{- range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}} 
  -|
   {{- range $i, $version := .VERSIONS | splitLines -}}
         dgoss run ...
   {{end}}

Does this not have the same issue that the sh case got in that you need to make sure white-space control is treated correctly to get the newlines in the right space? What if I want to add a command in top of the for loop?

template:
   -|
     echo hi there
     {{- range $i, $version := .VERSIONS | splitLines -}}
         {{ if $version }}docker build -t "{{ $.NS }}/{{ $.REPO }}:{{ $version }}" ./context/{{ $version | replace "-" "/" }}{{ end }}
      {{ end -}} 

What do you feel about the range cmd/sh keyword suggestion?

@twhiston
Copy link
Author

twhiston commented Dec 19, 2017

@smyrman yeah, newlines will always be a bit of a nightmare i guess, personally i would add that echo as a seperate template slice entry, as it is not doing anything dynamic (or i would allow cmd and template and merge them together so you could put that command in the cmd) but you are totally right that we could not stop anyone doing it as above, and then yeah new line nightmare again. Also i guess we have that problem if people split over lines for readability but use the whitespace controls to ensure it is all output in a single line when parsed.

the range cmd/sh seems like a cool solution, but if range could be a var that would be even better , i'm thinking for example that you could create a very simple resuable task file for building and testing docker images if you deal with the dynamic parts in the vars file and not in the tasks, this is actually the way we currently deal with building docker images using make, super generic targets and project specific vars file included

@smyrman
Copy link
Contributor

smyrman commented Dec 19, 2017

but if range could be a var that would be even better

I was presuming we allowed `range: "{{.MYVAR}}". Then you have the option to do both.

 for building and testing docker images if you deal with the dynamic parts in the vars file and not in the tasks

A bit of-topic, but we use templated tasks for this. I think I can share at least some of it, with some container names sensored if you are interested.

@twhiston
Copy link
Author

I was presuming we allowed `range: "{{.MYVAR}}".

yeah i think you are totally right. If we are in agreement about this i'd like to offer again to do the implementation for it :)

I think I can share at least some of it

i'd be really interested to see those templated tasks if its possible to share them, thanks very much!

@smyrman
Copy link
Contributor

smyrman commented Dec 20, 2017

If you are interested in doing a PR, that would be great! There are some tricky bits with evaluation order / variable expansion that you would need to be aware of as I believe templates in vars (and cmds) are currently expanded before the task is started.

The best thing might be to alter the method that does this expansion to expand a range to multiple CMDS before issuing the command, and also evaluate the (range) template variables within that method...

Don't be afraid to start the PR before you are done (marked as WIP) to get early feedback, or to post some ideas on how to attack the problem here or on gopher slack.

i'd be really interested to see those templated tasks if its possible to share them, thanks very much!

Here they are. These are currently in the state of rewrite, and I want to share the latest and greatest so the exact version in the gist have not been tested, but we have something very similar that works. https://gist.github.com/smyrman/fdc43a72168ec7af16c2e35600c465e9. Feel free to ping me on gopher-slack if you have any questions.

@andreynering
Copy link
Member

andreynering commented Dec 20, 2017

Just to feed the brainstorming, not rejecting any other idea. Something I has in mind for #29 is:

echo:
  sources: **/*.txt # foo.txt, bar.txt, foo/bar.txt
  foreach: true
  cmds:
    echo "{{.SOURCE}}"
task echo
# echo foo.txt
# echo bar.txt
# echo foo/bar.txt

So, maybe we can do something like this:

# will run for each source file
foreach: source
# will run for each variable value
foreach: {var: VAR, split: " "}

@twhiston
Copy link
Author

@andreynering @smyrman Sorry for the delay, took a bit of a break over Christmas. As you have made some progress on #29 and #31 does that change your thoughts on how to implement this? I'll actually make a pass at doing this over the coming weeks so would be good to get a quick update.
@smyrman thanks for the templated task files example!

@andreynering
Copy link
Member

andreynering commented Feb 18, 2018

@twhiston Sorry for the long time to respond you. I had other priorities in the issue list, and I'll slowly start to think about this again.

@twhiston
Copy link
Author

@andreynering no problem at all, happy to discuss it whenever you have time, no rush :)

@andreynering andreynering added help wanted Issues that could benefit from members of the community contributing their expertise or experience. type: feature A new feature or functionality. labels Feb 18, 2018
@andreynering andreynering added this to the v2 milestone Feb 18, 2018
@andreynering
Copy link
Member

@twhiston I think a good start is looking to the foreach syntax I proposed in this comment above (or similar).

But if you have issues or better ideas for the syntax, we can always discuss in a possible PR. (If you want, open PR as WIP and ask for feedback).

Thanks 😄

@andreynering andreynering changed the title templating and command lists Templating, command lists and file globs Feb 18, 2018
@andreynering andreynering modified the milestones: v2, v2.1 Mar 3, 2018
@andreynering andreynering modified the milestones: v2.1, v2.x Mar 13, 2018
@AlvarBer
Copy link

Any updates on the foreach syntax?

@andreynering
Copy link
Member

@AlvarBer Nothing for now, sorry

@ghostsquad
Copy link
Contributor

I have first-class ideas for how to implement this in v4

@andreynering
Copy link
Member

@ghostsquad Once you're done to write a few examples of what you have in mind, that would certainly be interesting. 🙂

@ghostsquad
Copy link
Contributor

@andreynering and others, a recent discussion that can provide some insight into this is here:
#700 (comment)

@andreynering
Copy link
Member

Work is in progress for this feature at #1220.

@andreynering andreynering removed the help wanted Issues that could benefit from members of the community contributing their expertise or experience. label Jul 25, 2023
@andreynering
Copy link
Member

andreynering commented Jul 25, 2023

It has been almost 6 years, but a loop feature was finally just released thanks to @pd93 work!

https://taskfile.dev/usage/#looping-over-values

ItsBeen84Years

@marverix
Copy link

I don't know if it's positive or negative that it was waiting for 6 years to implement :) I know that it is an OSS project so it is how it is, but still, probably something discouraging to start using Task in the organization and/or consider sponsorship.

@andreynering
Copy link
Member

@marverix We had a lot a activity throughout these 6 years. Yet, some features take some time to happen, like the loop feature we just released.

Keep in mind that all maintainers and contributors have days jobs and a life outside GitHub. We have few hours a week to dedicate to this project. So, that some things needs time to happen doesn't bother me, we do what we can.

Whether you are going to adopt Task or not it's your decision. I regularly review PRs, so if you want to contribute to a given feature, that's a possibility as well.

@marverix
Copy link

@andreynering Sorry if I sounded arrogant. That wasn't my goal. As a developer who is also active in the Open Source world, I know what you are talking about. I meant that you can laugh at such things privately on the Discord channel. So it was more of a sincere concern.

About contribution: I would love to, but not everyone can write in Golang. But, I will try my best to figure out how to write something on my own.

Sorry again,
Br
Marek

@andreynering
Copy link
Member

I meant that you can laugh at such things privately on the Discord channel. So it was more of a sincere concern.

I got the point now, thanks! I just think a bit of humor is needed in life. 🙂

And I didn't find you arrogant, just wanted to make clear why things don't always move as fast as some people may want.

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

Successfully merging a pull request may close this issue.

6 participants