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 task hooks #667

Closed
wants to merge 5 commits into from
Closed

Conversation

tylermmorton
Copy link
Contributor

@tylermmorton tylermmorton commented Jan 29, 2022

This PR is in response to a series of issues: #141, #204 and #344. This code adds "hooks" as a feature, with the initial implementation containing 5 hooks. I've added unit tests to cover all of the new behavior, and additional documentation containing examples of each hook!

  • before_all commands are called before a task starts execution
  • after_all commands are called after a task completes, success or failure
  • on_success commands are called when a task completes successfully
  • on_failure commands are called when a task fails due to an exit code other than 0
  • on_skipped commands are called when a task is skipped due to status, precondition or checksum

Here is an example of the usage:

version: '3'

tasks:
  notify:
    vars:
      STATUS: "NONE"
    cmds:
      - curl -d "status={{.STATUS}}" -X POST http://webhook.example.com
      
  test:
    cmds:
      - go test
    hooks:
      on_success:
        - task: notify
          vars:
            STATUS: "PASS" 
      on_failure:
        - task: notify
          vars:
            STATUS: "FAIL"

@tylermmorton tylermmorton requested review from andreynering and kerma and removed request for andreynering January 29, 2022 21:47
@tylermmorton
Copy link
Contributor Author

tylermmorton commented Jan 29, 2022

I'm running into issues in VS Code where the Taskfile YAML Schema is complaining about the new hooks field. Any ideas on how to update this?
image

Edit: Nevermind, found this ... I will link the pull request here when done

@tylermmorton tylermmorton changed the title Task hooks Add task hooks Jan 29, 2022
@andreynering
Copy link
Member

andreynering commented Jan 31, 2022

Hi @tylermmorton, thanks for yet another PR.

I see future in this PR, but I understand we need to discuss what we really want to do here before proceeding.

The idea of wrapping setting like this into hooks is probably interesting, making it consistent with each other and easy to add more in the future.

The on_failure and on_skipped hooks are likely very useful.

The before_all and on_success feels not needed to me initially, since one could just add more commands to the beginning or end of a task.

In a similar manner, the after_all duplicates functionality we recently released with the defer keyword, so I'd be inclined to remove this one as well.

Regarding #204 you linked, the idea of a possible setup: keyword is that it would be global, so it would kinda run before every declared task. We could consider turning that into a kind of hook, but that would means we would have two kind of hooks, global and local (task-specific) ones.

EDIT: Still about #204, not only running before, and also keeping state like declared functions and environment variables, etc. that would still be accessible inside tasks. So this is actually more than just a hook.


This is the kind of feature where the input from user is very important. So if whoever is reading this would like to add thoughts, they're more than welcome!

@andreynering andreynering added type: feature A new feature or functionality. proposal labels Jan 31, 2022
@tylermmorton
Copy link
Contributor Author

tylermmorton commented Feb 2, 2022

Hey @andreynering, thanks the feedback! :)

I'd like to make the argument to keep after_all as a hook because of the way that defer works. There's some nuance there that isn't completely apparent to users not familiar with Go. To me, users that are already using other hooks to run commands are naturally going to reach for after_all instead of defer.

I agree that before_all and on_success are mostly redundant hooks and I only added them to give the feature some symmetry. If there's an on_failure hook it seems natural to me to have an on_success hook as well. Users defining tasks using hooks might appreciate the semantics that even these redundant hooks provide.

I will say to implement and maintain a hook is much easier than other features, as a hook in the current implementation does not control the flow of or branch any of the task logic. Every new hook definition is roughly 12 lines of code, which could be reduced, especially all of the boilerplate within executor's hooks.go. The call to a hook is just a one-liner!

--

I just read deeper into #204 and I guess I did not get far enough into the discussion to realize this PR does not cover that 😅 My bad. I do really like the idea of a global setup hook that can modify the environment tasks are run in. I'm actually trying to solve this exact problem on one of my projects and the idea of using source and a setup script to build the shell environment seemed like a great solution. There's some value in being able to do that within the Taskfile to help keep all of my team's IaC centralized.

I can probably get some code in for this next time I'm able to pull up my IDE. I will take any notes I have to that issue thread instead of this PR. Would it be easier for you to fold that feature into this branch or should I add the implementation separately?

@andreynering andreynering mentioned this pull request Apr 16, 2022
@ghostsquad ghostsquad added the v4 label May 9, 2022
@pd93 pd93 removed the v4 label Oct 15, 2022
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 this pull request may close these issues.

4 participants