test: update add_vagrant_box.sh

[twpayne]

This commit fixes a few portability issues and minor bugs, mainly caught
by ShellCheck.

[qmonnet]

Could you please elaborate on this change? Asking because I was under the impression that printf was more portable than echo. Or is it because you just want a newline character at the end of the file?

[twpayne]

Ah, good catch with the newline. I initially switched away from printf as there were no arguments being passed, and correction functionality required $box to not contain any printf verbs (e.g. if $box was foo%s then printf would fail, of course this would not likely happen in practice).

The relevant ShellCheck is SC3037, which indeed warns that flags to echo are undefined in POSIX sh. However, this script explicitly uses /bin/bash, and -n is defined there.

Result: I've added a -n to echo.

[qmonnet]

Ok, I was just wondering why the switch. I understand the %s reason. Thanks!

I don't think the newline matters at all, and I think we'd be as well without this -n option. It's true we enforce bash so we should have it, but it's just not clear when reading the script what we need to omit the new line (I suspect printf didn't add it just because nobody cared to add it). But that's a minor details anyway, so feel free to revert the -n or to keep it :).

[twpayne]

Vagrant itself seems to omit the trailing newline, so let's keep the -n.

[christarazi]

LGTM pending Quentin's comment

[twpayne]

test-me-please

[qmonnet]

This script is not used in CI tests.

[twpayne]

Ah, thanks. I saw references to it in several Jenkinsfiles:

jenkinsfiles/ginkgo-kernel.Jenkinsfile:102:                sh '/usr/local/bin/add_vagrant_box ${WORKSPACE}/${PROJ_PATH}/vagrant_box_defaults.rb'
jenkinsfiles/ginkgo-runtime-kernel.Jenkinsfile:73:                sh '/usr/local/bin/add_vagrant_box ${WORKSPACE}/${PROJ_PATH}/vagrant_box_defaults.rb'
jenkinsfiles/kubernetes-upstream.Jenkinsfile:65:                sh '/usr/local/bin/add_vagrant_box ${WORKSPACE}/${PROJ_PATH}/vagrant_box_defaults.rb'

but see now that these are in fact provisioned as part of the Packet Terraform scripts.

[pchaigno]

We should really move this script to contrib/ to clarify that and fix the codeowners in one go. I've stopped the Jenkins builds.

[qmonnet]

We should really move this script to contrib/ to clarify that and fix the codeowners in one go.

-> #15859