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

Linting of commands #701

Open
orenmazor opened this issue Apr 14, 2022 · 8 comments
Open

Linting of commands #701

orenmazor opened this issue Apr 14, 2022 · 8 comments
Labels
good first issue Issues that are good for first-time contributors to pick up. help wanted Issues that could benefit from members of the community contributing their expertise or experience. type: enhancement A change to an existing feature or functionality.

Comments

@orenmazor
Copy link

So this is not fully fleshed out in my head yet.

The gist is as such: my team currently uses makefiles + shell scripts to do things. this is obviously messy and awful.

I've done a few PoCs to migrate those things into taskfile and it's a beautiful world.

but here's the rub: we use shellcheck in all of our CI/vim/local machines/etc to verify shell scripts for various code smells.

I'd love to figure out a way to do this in taskfile, but I haven't used this tool enough yet to figure out how to do it.

I can think of a few hacky ways to do it. for example: task --shell -t mytaskfile.yaml mytask and it dumps that task as a mini-shell script. I can then pipe that to shellcheck.

or even better, build shellcheck into task: task --lint -t mytaskfile.yaml mytask and it would run each command through shellcheck.

the goal isn't to verify anything or do any complex stuff. the goal is to catch weird little typos or bad commands or other basic tripwire features.

@andreynering andreynering added type: enhancement A change to an existing feature or functionality. help wanted Issues that could benefit from members of the community contributing their expertise or experience. good first issue Issues that are good for first-time contributors to pick up. labels Apr 14, 2022
@andreynering
Copy link
Member

Hi @orenmazor,

You seem to work for 1Password? I use that for both work and personal use and love the tool. 🙂


Interesting idea, indeed. I'd say it's a bit unconventional for Task to have official integrations with external tools, although integrations with watchman and Docker were discussed in the past. Go stuff usually try to be self-contained when possible.

For this reason, it'd make more sense to try to do that with mvdan/sh given we already use it for actually running stuff. The problem is, it only contains a formatter, and not a linter AFAIK.

To allow shellcheck use it'd make more sense to have a flag to output the bash so it can be manually redirected to shellcheck (or something else), as you suggested. This is probably relatively simple, and pull requests would be welcome!

@orenmazor
Copy link
Author

hey Andrey, sure do. we're testing this tool out in our ops teams :)

and cool, I agree with you. a pure bash output would be the handiest as far as integrations go. I'll kick this idea around and maybe punt a PR at you folks :)

@perobertson
Copy link

Having taskfile output the contents of the bash script would work great. That way each tool can work on their own without coupling them together. In my example here, we could replace the echo "echo 'hello'" with the task command that would output the shell script.

shellcheck <(echo "echo 'hello'")

In /proc/self/fd/11 line 1:
echo 'hello'
^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

@ghostsquad
Copy link
Contributor

I actually really like the idea of shellcheck native integration. I think that would be very clean, and the fact that you can use shell scripts in many places throughout a task file would make it cumbersome to validate. I'm going to tag this, and come back to it for v4.

@ghostsquad ghostsquad added the v4 label Apr 16, 2022
@andreynering
Copy link
Member

@ghostsquad Let's proceed with the idea discussed above of having flag to output the command first. That's simple enough and allow integration with other tools as well, not being tied to shellcheck only. I also don't think we need to wait for v4 to have it.

I think that will be good enough as a first try, and we can always decide to have a better integration in the future if that shows necessary.

@ghostsquad
Copy link
Contributor

Ya, no need to wait for v4, just keeping this on my list.

@the-kaustubh
Copy link

please assign the issue to me. Thanks!

@KostasKgr
Copy link

Being able to print the final contents of the commands would be useful during development. Some times I might be creating complex taskfiles where it's not obvious that they will work how I think they will.

This could also be useful during testing, one could assert on the final contents of the commands when working with complicated vars, or with yaml anchors etc.

I was going through the latest documentation and saw the --dry mode. This seems to satisfy the requirements, apart, perhaps, from the way the output is formatted.

I made an example taskfile:

version: '3'

vars:
  TF_IMAGE: hashicorp/terraform:1.5.3
  TERRAFORM: docker run -t -e POWERDNS_API_KEY -v {{ .ROOT_DIR }}:/tf --rm {{ .TF_IMAGE }} -chdir=/tf/src/app1


tasks:
  terraform:
    desc: "Run terraform using docker"
    dir: src/app1
    requires:
      vars:
        - TF_PASSWORD  # You must export TF_PASSWORD to be a gitlab access token
        - TF_USERNAME  # You must export TF_USERNAME
    cmds:
      - echo "This is a different cmd!"
      - |
        {{if eq .CLI_ARGS "init"}}
            {{.TERRAFORM}} init -backend-config=username=${TF_USERNAME} -backend-config=password=${TF_PASSWORD} -reconfigure
        {{else}}
            {{.TERRAFORM}} {{.CLI_ARGS}}
        {{end}}

  cmdscope:
    cmds:
      - export MY_VARIABLE=123456
      - echo $MY_VARIABLE

Output

> task --dry terraform -- init
task: [terraform] echo "This is a different cmd!"
task: [terraform] 
    docker run -t -e POWERDNS_API_KEY -v /home/myuser/dry_mode:/tf --rm hashicorp/terraform:1.5.3 -chdir=/tf/src/app1 init -backend-config=username=${TF_USERNAME} -backend-config=password=${TF_PASSWORD} -reconfigure
task --dry terraform -- plan --refresh
task: [terraform] echo "This is a different cmd!"
task: [terraform] 
    docker run -t -e POWERDNS_API_KEY -v /home/myuser/dry_mode:/tf --rm hashicorp/terraform:1.5.3 -chdir=/tf/src/app1 plan --refresh

Each cmd entry is a separate script, for example cmdscope will not print 123456. So I assume linting should be done per cmd list element. It's not obvious to me that there is an output format that would directly work as a pipe to shellcheck.

If the scripts are complicated perhaps they should be different files altogether? Then one could shellcheck the scripts/* contents and the task file would manage calling them from the right folders with the right environment variables / secrets etc.

Note that --dry seems to run the sh commands of env. Is there a way that those can influence the cmds?

In summary, the --dry seems to provide the means for an external integration with shellcheck.

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. help wanted Issues that could benefit from members of the community contributing their expertise or experience. type: enhancement A change to an existing feature or functionality.
Projects
None yet
Development

No branches or pull requests

7 participants