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

Override silent option when calling other tasks #680

Closed
HeCorr opened this issue Mar 2, 2022 · 13 comments · Fixed by #1142
Closed

Override silent option when calling other tasks #680

HeCorr opened this issue Mar 2, 2022 · 13 comments · Fixed by #1142
Assignees
Labels
good first issue Issues that are good for first-time contributors to pick up.

Comments

@HeCorr
Copy link
Contributor

HeCorr commented Mar 2, 2022

This does not as expected:

tasks:
  notsilent:
    cmds:
      - echo "hey"

  maybesilent:
    cmds:
      # maybe some other tasks here, which I may not want to be silent
      - task: notsilent
        silent: true

image

For comparison, this is what I expected to see:

image

It would be really good if it worked, for the cases where one might want the called task to not be silent when executed directly, but be silent when executed from another task.

I know there's the -s flag, but it wouldn't work for the case where one might want the other cmds to not be silent.

@kerma kerma added the type: feature A new feature or functionality. label Mar 2, 2022
@mknapik
Copy link

mknapik commented Mar 4, 2022

I had a similar use case. I want to run have a task for pull docker containers with optional ignore_error (e.g. pulling required docker images vs pulling CI build cache).

I tried to make it work with vars

  docker-pull:
    run: when_changed
      - cmd: docker pull {{.DOCKER_IMAGE}}
        ignore_error: '{{.IGNORE_ERROR | default false}}'

The solution above was silently failing. It seems ignore_error expects boolean and fails silently when string (e.g. "false") is passed.

When using ignore_error on the task level:

  docker-pull:
    run: when_changed
    cmds:
      - docker pull {{.DOCKER_IMAGE}}
    ignore_error: '{{.IGNORE_ERROR | default false}}'
    silent: '{{.SILENT | default false}}'

I get an type error:

yaml: unmarshal errors:
  line 275: cannot unmarshal !!str `{{.IGNO...` into bool

@mknapik
Copy link

mknapik commented Mar 4, 2022

#363 similar functionality, but for ignore_error.

@4x0v7
Copy link

4x0v7 commented Jul 28, 2022

I have a similar use case, but using includes:

When the calling taskfile has silent: false at Task level:

version: '3'
silent: false

If an included taskfile has:

version: '3'
silent: true

Then output is displayed, where I expect the included files silent to take precedence.

Swapping them over; calling task is silent: true, called task is silent: false, then there is no output.

I'm not sure how the silent option should propagate with includes: tasks

@andreynering andreynering added the good first issue Issues that are good for first-time contributors to pick up. label Aug 4, 2022
@danquah
Copy link
Contributor

danquah commented Apr 5, 2023

I'm considering looking at this after #546

It feels like something where it makes sense to handle both silent and ignore_error in the same go. Any opinions @andreynering ?

As far as I can tell it will require us to widen the Call interface used for calling a task. Seem like this is the place where a Task calls another Task. The change seems simple on the surface, but it also feels like a change that needs to be implemented more than one place ...

@andreynering
Copy link
Member

Hey @danquah,

Feel free to take this issue.

Yes, it does indeed makes sense implement both silent and ignore_error at the same time, and yes, the Call struct is the starting point to implement this.

I believe it should be a relatively simple thing to implement.

@danquah
Copy link
Contributor

danquah commented Apr 6, 2023

Sounds good - I'll probably work on it this coming week

@danquah
Copy link
Contributor

danquah commented Apr 13, 2023

Done some initial work on the issue now. The final changes are not going to be too complicated, but there are a couple of important decisions that has to be made

  1. It's all well and fine to introduce a new flag that is passed via Call when we do a RunTask, but we also have to do something similar when the "root" call is created - that is, if silent is enabled at the Taskfile level, that flag should be added to the initial Call instance(s) and inheritance should do the rest of the work. This is probably the way to go, and will mean that it is no longer necessary to check a top-level flag. We'll see.

  2. I realised that while fixing inheritance of silent is pretty much that - a fix of something that is not 100% consistent right now - inheriting ignore_error will potentially have bigger consequences. I might be assuming a bit here, but silencing output rarely breaks anything. Suddenly ignore errors on the other hand could cause problems. In both cases we can probably argue that we're just making Task more consistent - but the changes feels different. So, it feels like it would make the most sense first to implement the low-stakes silent inheritance, use that to get the flag in place, and then in a later PR expand with a ignore_errors flag.

None of these are blockers, I'll chuck along - but if you have time I would appreciate a yay/nay on whether hooking this in already in up in ParseV3 is the way to go, and whether you agree with ignore_error being a breaking change and thus going for a two-phase change is better @andreynering

@andreynering
Copy link
Member

Hi @danquah,

Some comments:

I think that NOT having inheritance for these flags would probably be more consistent with how Task work overall. ignore_error: true would make that specific task to have errors ignored, but not for extra task calls made by that task. This also avoid any backwards-incompatibility. This is one of these tricky scenarios where every user will have a different expected behavior, so it's hard to decide, but my inclination is to avoid inheritance.

I agree that working on silent first would be better. The same question applies, but it's not backwards-incompatible to change later if needed, as you said. I think I'm included to make it NOT inherit, though. If the user wants to silence everything, they can already add to the top-level Taskfile to use --silent.

@danquah
Copy link
Contributor

danquah commented Apr 26, 2023

Still working on this - getting close, I have a quick question. First a quick reflection:

The logic for when something should go silent starts to get a bit complex. I it's ok as long as you just stick to a single strategy for how to silence things - but when you mix the options up things starts to get complicated. The thing that confused me the most even during implementation is the difference between silencing Task (ie, stop outputting "task: [some task] some command") and silencing the actual output from the command (ie, stop output from "echo 'hello world'"). We support silencing task itself, but not silencing the commands.

OK, now for the question.

What takes precedence here?

  task-test-chatty-calls-silenced-cmd:
    silent: false
    cmds:
      - cmd: exit 0
        silent: true

I'm thinking the silent on the cmd so that in the end the task won't output anything.

@pd93
Copy link
Member

pd93 commented Apr 26, 2023

Just my opinions and by no means the final answers:

We support silencing task itself, but not silencing the commands.

This makes sense to me. Task is adding additional text to the output from commands which might not always be desirable. We need to be able to disable this independently of silencing command output. Silencing command output is already a feature of most (all?) shells using redirection. e.g. echo foo &> /dev/null. I don't believe there is any need for Task to add this functionality too.

What takes precedence here?

I agree with you that the command should take precedence. We should override when being more specific. It would be very confusing to have silent: true, but still print the task output. If other commands were added to the task without specifying silent, I would expect them to inherit the property from the task (in this case, silent: false.

@andreynering
Copy link
Member

I agree with Pete comments.

@danquah
Copy link
Contributor

danquah commented Apr 27, 2023

Ok, draft PR time #1142

I'll finish off tomorrow by adding documentation of the new Silence field I've added to Dep - as I understand it it requires a change to both the Usage and API documentation and the json schema.

@danquah
Copy link
Contributor

danquah commented Apr 30, 2023

PR is ready for review now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are good for first-time contributors to pick up.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants