cirrus lib.sh: refactor req_env_var()#3053
Conversation
|
Sorry; I have no hints for reviewing other than suggesting the use of a good diff colorizer. |
cevich
left a comment
cevich
left a comment
There was a problem hiding this comment.
Thank you so much ed, this is WAY better than I could have done. Only small ask is you add running the unittest script to Makefile under the localunit target. That will give us coverage on all platforms and development environments.
There was a problem hiding this comment.
Oh good you caught this possible inf. loop 😄
There was a problem hiding this comment.
ooooohhhhhhh praise $DIETY thank theee for blessing @edsantiago to write this thy unittest thanks be the $CI
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cevich, edsantiago, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
LGTM pending tests pass with something like this: diff --git a/Makefile b/Makefile
index 9228ec71..cf4e9364 100644
--- a/Makefile
+++ b/Makefile
@@ -188,6 +188,7 @@ localunit: test/goecho/goecho varlink_generate
--tags "$(BUILDTAGS)" \
--succinct
$(MAKE) -C contrib/cirrus/packer test
+ bash contrib/cirrus/lib.sh.t
ginkgo:
ginkgo -v -tags "$(BUILDTAGS)" $(GINKGOTIMEOUT) -cover -flakeAttempts 3 -progress -trace -noColor -nodes 3 test/e2e/.Other option is to add it to |
Existing code was not working due to a bash gotcha ('exit'
from a pipeline). It also had unnecessary duplication.
New version is safer; also includes unit tests run under localunit.
Existing invocations of req_env_var replaced via:
$ [ edit setup_environment.sh, move one closing quote to its own line ]
$ perl -ni -e 's/(?<=req_env_var )"(\S+)\s+\$\1"/$1/; if (/req_env_var "$/ .. /^\s*"/) { chomp; s/(?<=\S)\s.*//; if (/^\s*"/) { print "\n" } else { unless (/req_env_var/) { s/^\s+//; print " ";} print;} } else { print }' $(ack -l req_env_var)
$ [ hand-massage an incorrect instance of '@' in lib.sh:ircmsg() ]
Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago
force-pushed
the
fix_req_env_var
branch
from
2b99eb5 to
295c531
Compare
|
Force-pushed with requested Makefile change. Thanks for the suggestion. |
|
@edsantiago fantastic, thanks. LGTM @mheon may we fast-track this one please, it adds some sorely needed fixes AND actual CI-unittests for a heavily used function in our Cirrus scripts. It's also needed for my #2561 |
|
/me sprinkles some go-faster and run-clean dust onto the automation pixies |
|
My bash-fu is very weak, but this LGTM |
TomSweeneyRedHat
left a comment
TomSweeneyRedHat
left a comment
There was a problem hiding this comment.
LGTM assuming happy tests
|
Thanks Matt and Tom. /lgtm |
|
@mheon (and future maintainers) the key background for this is that #2561 is making some changes to various invocations of the |
github-actions
Bot
added
the
locked - please file new issue/PR
6 participants
Existing code was not working due to a bash gotcha ('exit'
from a pipeline). It also had unnecessary duplication.
New version is safer; also includes unit tests.
Existing invocations of req_env_var replaced via:
$ [ edit setup_environment.sh, move one closing quote to its own line ]
$ perl -ni -e 's/(?<=req_env_var )"(\S+)\s+$\1"/$1/; if (/req_env_var "$/ .. /^\s*"/) { chomp; s/(?<=\S)\s.//; if (/^\s"/) { print "\n" } else { unless (/req_env_var/) { s/^\s+//; print " ";} print;} } else { print }' $(ack -l req_env_var)
$ [ hand-massage an incorrect instance of '@' in lib.sh:ircmsg() ]
Signed-off-by: Ed Santiago santiago@redhat.com