Add support for extended and recursive globs #42

mckern · 2022-09-14T01:47:04Z

Ran into #35, and recursion would be extremely useful for my org. So I've added it.
I figure that extglob and globstar are pretty safe shopts since Bash 4 came out in 2009, but I've left some "safety valves" to ensure that we don't try to set those options against Bash 3 if we run into it.

Oh, I also ran shfmt and shellcheck against my work, so if there's little bits of whitespace drift those were introduced by tooling and they're safe.

Ensures that Bash version 4 is at least 4 before enabling it.

Also adds tests to validate that these options behave as expected.

Matching shell options against versions is good but since only two options are exposed through the plugin config, we only need to check for extglob and globstar.

mckern · 2022-09-14T01:50:42Z

Noticed that there's no CI wired up to this plugin (which is really funny in context), but the tests passed for me when I ran them locally:

$ docker run -it --rm -v "$PWD:/plugin:ro" buildkite/plugin-tester
run.bats
 ✓ Shellcheck a single file
 ✓ Shellcheck multiple files
 ✓ Shellcheck multiple files with starglob
 ✓ Shellcheck multiple files with extglob
 ✓ Shellcheck a single file with single option
 ✓ Shellcheck a single file with multiple options
 ✓ Shellcheck multiple files with multiple options
 ✓ Shellcheck failure

8 tests, 0 failures

I'll try running these in a bit against my fork.

pzeballos · 2022-09-14T20:57:33Z

Hey @mckern! This is really good! 🎉 We only noticed two things:

When you set either option to false, options are still enabled. You should also add tests for these cases.
Just to make sure the options are only turned on for this particular command (and it doesn't bleed out of scope) you should revert the option back to what they were before the end of the hook.

Thanks!

mckern · 2022-09-14T21:10:06Z

@pzeballos can-do. I'll push up some commits after lunch. Thanks for taking a look.

When you set either option to false, options are still enabled. You should also add tests for these cases.

Oof, because they're defined as booleans, they're being rendered out as 1 or 0 in the inherited environment variable aren't they? That's on me for assuming they'd just be unset -- the test was only for null/not-null strings.

Just to make sure the options are only turned on for this particular command (and it doesn't bleed out of scope) you should revert the option back to what they were before the end of the hook.

I'd assumed that each stage of a pipeline run happens within its own sub-shell (with its own scoped environment), and there's only one command stage in a run; can this bleed through the post-command stages?

Environment variables defined as boolean values in the spec will be cast to "1" or "0", so we should evaluate for value instead of just evaluating for presence. This also adds test cases, exercising what happens when each option is explicitly disable.

Previous fix was operating under a false assumption; after doing a little debugging, the Buildkite Agent expresses these values as the strings "true" or "false". Our test should take that into account. This is implied by json-schema.org's guidlines for boolean values but it is not made clear in the Buildkite plugin documentation.

It looks like the values of these options are passed through literally as environment variables with no casting whatsoever done. So we need to accommodate for "true" and "1" and case insensitivity when figuring out if shopts should be set. There's some test cases to flex the negative scenarios, as well as the positive.

mckern · 2022-09-15T00:33:55Z

@pzeballos test cases for enabled and disabled are updated and logic now accommodates the soft nature of booleans as expressed via Bash environment variables.

Regarding resetting shopt values after the command hook is run, I built a test branch with a post-command hook to evaluate shopt values and they appear to be contained to the singular command stage. Since there can only be one command hook (unlike other stages), that seems sufficient to prevent shopt inheritance between stages to me -- is this a scenario you've encountered before or is this more of a "let's try to cover all the bases" scenario?

pzeballos · 2022-09-15T04:18:39Z

is this a scenario you've encountered before or is this more of a "let's try to cover all the bases" scenario?

indeed, it was that. We'll review your update and let you know. Thanks for the quick turnaround @mckern!

hooks/command

tests/run.bats

Co-authored-by: Pol <pzeballos@users.noreply.github.com>

`extglob` was introduced in Bash 2, which had its last release in 2002. It's a pretty safe target, so we don't need a note about it.

Instead of testing against a version lookup, we can just try setting the shopt directly and handling the failure ourselves. It's a little more brute-force but it's also a little bit less dependent on internal Bash variables. The nature of shopts means we can't directly test them without overriding so much code that we're essentially using a function to exercise a test (not the other way around). But there's ample comments describing the important details, like "no major platform ships bash 3 except macOS".

plugin.yml

Instead of directly exposing implementation details by using Bash shopts as config keys, use the nicer `extended_glob` and `recursive_glob` keys. Evaluating for the expected variables moves out into the main script body, and casting the variable values to boolean values is now its own function. Setting either value will also now emit an explicit message to stdout, underneath their own (de-emphasized) build-phase grouping. A test has been added to validate that expected behavior still applies when both options are enabled at the same time.

toote

Looking great... but we have fixed the tests in the master branch, which means your tests are now failing (the same way they should have been failing before).

I tried making the change myself but I didn't have permissions to do so. After the mergeback from this is the diff:

diff --git a/tests/run.bats b/tests/run.bats
index cabec31..c1fdad6 100644
--- a/tests/run.bats
+++ b/tests/run.bats
@@ -43,7 +43,7 @@ load '/usr/local/lib/bats/load.bash'
   export BUILDKITE_PLUGIN_SHELLCHECK_FILES="**/*.sh"
 
   stub docker \
-    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always \"tests/testdata/recursive/subdir/stub.sh tests/testdata/test.sh tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell with space.sh\"' : echo testing stub.sh test.sh llamas.sh shell with space.sh"
+    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always tests/testdata/recursive/subdir/stub.sh tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell\ with\ a\ space.sh tests/testdata/test.sh : echo testing stub.sh test.sh llamas.sh shell with space.sh"
 
   run "$PWD/hooks/command"
 
@@ -59,7 +59,7 @@ load '/usr/local/lib/bats/load.bash'
   export BUILDKITE_PLUGIN_SHELLCHECK_FILES="tests/testdata/subdir/*.+(sh|bash)"
 
   stub docker \
-    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always \"tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell with space.sh tests/testdata/subdir/stub.bash\"' : echo testing llamas.sh shell with space.sh stub.bash"
+    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell\ with\ a\ space.sh tests/testdata/subdir/stub.bash : echo testing llamas.sh shell with space.sh stub.bash"
 
   run "$PWD/hooks/command"
 
@@ -106,7 +106,7 @@ load '/usr/local/lib/bats/load.bash'
   export BUILDKITE_PLUGIN_SHELLCHECK_FILES="**/*.+(sh|bash)"
 
   stub docker \
-    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always \"tests/testdata/recursive/subdir/stub.bash tests/testdata/subdir/stub.bash tests/testdata/recursive/subdir/stub.sh tests/testdata/test.sh tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell with space.sh\"' : echo testing stub.sh test.sh llamas.sh shell with space.sh"
+    "run --rm -v $PWD:/mnt --workdir /mnt koalaman/shellcheck:stable --color=always tests/testdata/recursive/subdir/stub.bash tests/testdata/recursive/subdir/stub.sh tests/testdata/subdir/llamas.sh tests/testdata/subdir/shell\ with\ a\ space.sh tests/testdata/subdir/stub.bash tests/testdata/test.sh : echo testing stub.sh test.sh llamas.sh shell with space.sh"
 
   run "$PWD/hooks/command"

toote · 2022-09-28T17:39:14Z

Noticed that there's no CI wired up to this plugin (which is really funny in context)

Actually, it is wired, but turned off for third-party forks

Co-authored-by: Matías A. Bellone <matias.bellone@builkite.com>

mckern · 2022-09-28T17:55:28Z

@toote thanks for the heads up; I applied your diff and committed it with you as co-author.

toote

Worked like a charm :)

hooks/command

pzeballos

Thank you for your contributions and bearing with us! 🙏🏻

mckern added 3 commits September 13, 2022 18:43

Add support for globstar

424eb40

Ensures that Bash version 4 is at least 4 before enabling it.

Make extglob and globstar configurable

1617d38

Also adds tests to validate that these options behave as expected.

Cut bash_version_check() back a little bit

fe25f3f

Matching shell options against versions is good but since only two options are exposed through the plugin config, we only need to check for extglob and globstar.

Fix typo in README

2aff1dd

mckern added 3 commits September 14, 2022 16:25

pzeballos requested changes Sep 21, 2022

View reviewed changes

hooks/command Outdated Show resolved Hide resolved

tests/run.bats Outdated Show resolved Hide resolved

tests/run.bats Outdated Show resolved Hide resolved

mckern and others added 5 commits September 21, 2022 17:54

Update tests/run.bats

0b527e1

Co-authored-by: Pol <pzeballos@users.noreply.github.com>

Update tests/run.bats

d5e82a7

Co-authored-by: Pol <pzeballos@users.noreply.github.com>

Remove version note for extglob

d054ae2

`extglob` was introduced in Bash 2, which had its last release in 2002. It's a pretty safe target, so we don't need a note about it.

typo fixup

fa74412

mckern commented Sep 22, 2022

View reviewed changes

plugin.yml Outdated Show resolved Hide resolved

toote linked an issue Sep 27, 2022 that may be closed by this pull request

Support recursive glob #35

Closed

toote requested changes Sep 28, 2022

View reviewed changes

Fix tests after PR #44

18c256b

Co-authored-by: Matías A. Bellone <matias.bellone@builkite.com>

toote approved these changes Sep 28, 2022

View reviewed changes

toote requested a review from pzeballos September 28, 2022 18:00

mckern commented Sep 28, 2022

View reviewed changes

hooks/command Outdated Show resolved Hide resolved

Fix typo

1145b7d

pzeballos approved these changes Sep 28, 2022

View reviewed changes

pzeballos merged commit 03732ed into buildkite-plugins:master Sep 28, 2022

Add support for extended and recursive globs #42

Add support for extended and recursive globs #42

Uh oh!

Conversation

mckern commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mckern commented Sep 14, 2022

Uh oh!

pzeballos commented Sep 14, 2022

Uh oh!

mckern commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mckern commented Sep 15, 2022

Uh oh!

pzeballos commented Sep 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toote left a comment

Choose a reason for hiding this comment

Uh oh!

toote commented Sep 28, 2022

Uh oh!

mckern commented Sep 28, 2022

Uh oh!

toote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pzeballos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mckern commented Sep 14, 2022 •

edited

Loading

mckern commented Sep 14, 2022 •

edited

Loading