purpleidea / mgmt

Next generation distributed, event-driven, parallel config management!
https://purpleidea.com/tags/mgmtconfig/
GNU General Public License v3.0
3.58k stars 311 forks source link

lang: funcs: add ExecFunc #688

Closed gelisam closed 1 year ago

gelisam commented 2 years ago

Runs a string as a shell command, then produces each line from stdout.

gelisam commented 2 years ago

This is a draft PR because the implementation is still a work-in-progress: I currently hardcode the command to be executed instead of taking it from the input stream.

gelisam commented 2 years ago

@purpleidea, how does this look like so far? Here are the things I currently plan to improve, anything else?

gelisam commented 2 years ago

As discussed offline,

gelisam commented 2 years ago

It was difficult to understand why seq 10 was working but ping -c 13 google.ca was not, and it turns out the stderr message was useful in figuring out why, so I'll also log stderr in order to help users debug their commands.

gelisam commented 2 years ago

@purpleidea this PR is now ready to review!

gelisam commented 2 years ago

CI says that the following tests are failing:

  1. ./test/test-headerfmt.sh
    The problem there is that the copyright notice I copied from lang/funcs/core/world/kvlookup_func.go was only going up to 2021, but it should be going up to 2022. I have now fixed this in lang/funcs/core/os/exec_func.go, but the problem remains in lang/funcs/core/world/kvlookup_func.go and many other files and I don't think this PR is the right place to fix all of those other files.
  2. ./test/test-commit-message.sh
    This one was tricky to investigate. Running

    ./test/test-commit-message.sh

    prints PASS on my machine, yet fails on CI. Why? It turns out CI sets some environment variables, GITHUB_SHA, GITHUB_REF, and GITHUB_BASE_REF, and that if those variables aren't set the test silently passes instead of complaining that they are not set. Thankfully, there is a comment in the script listing those three variables and giving an example of what they should be set to. Unfortunately, that example is wrong; it claims it should be GITHUB_REF=refs/heads/exec-func but that doesn't work, it must be GITHUB_REF=exec-func. Here is the correct incantation for this PR:

    GITHUB_SHA=ef63f74f306cd90c2c40718bdb5bff327ac9f4b2 GITHUB_REF=exec-func GITHUB_BASE_REF=master ./test/test-commit-message.sh

    After some more fussing trying to figure out which part of that complex regex was failing, I eventually figured out that it's not sufficient for my commit message to begin with lang: funcs: ..., the ... also has to start with a capital letter.

  3. ./test/test-govet.sh This one complains that my comment "is not reflowed properly", but doesn't tell me how to fix it. My comment is already wrapped to 80 characters, which is what the comments in ./test/comment_parser.go around that error message talk about, so I have no idea how I'm supposed to "reflow" my comment in order to make it match the test.
gelisam commented 2 years ago

Turns out I was wrapping at 79 characters instead of 80.

gelisam commented 2 years ago

@purpleidea ready for another review!

purpleidea commented 1 year ago

Your glorious patch has been renamed to use system instead of exec as per your permission. I also added a second patch to fix a go vet issue which was breaking the build. Sorry the builds were rotted a bit previously. I've now fixed all of these and the basic tests should be stable now. Cheers!