chore: introduce strict bash mode for scripts,.github

[fuweid]

According to Github Actions [doc][1], linux platform will use bash -e {0} mode to run script. But we also need pipefail and nounset in CI. This patch is aimed to add strict mode setting before any run.

And remove the | tee and egrep commands since we don't need this workaround to show error.

[1]: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell

REF: #15514

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[serathius]

Any reason to do it differently?

[fuweid]

Sorry for the late reply. Some scripts use this pattern, for instance https://github.com/etcd-io/etcd/blob/main/scripts/update_proto_annotations.sh#L6. I will change it into one line if the file doesn't run strict mode.

[serathius]

That's a lot of scripts changed, most of them are not tested on CI. Have you tested all of them manually? You don't need to migrate all scripts at once. We could split PR based on script type. Most important is ./scripts/tests.sh

[fuweid]

Have you tested all of them manually? You don't need to migrate all scripts at once. We could split PR based on script type. Most important is ./scripts/tests.sh

Yes. I'm still testing it in my local. Will update it later.~

[fuweid]

side note: fast fail return if there is any error from ./e2e/.... If not, test.sh just reports it fails but no error exit code.

[ahrtr]

If not, test.sh just reports it fails but no error exit code.

Could you explain this? Isn't it a bug? The script should exit on error, because of set -e

[fuweid]

The run_pass is using if to check the command result.

etcd/scripts/test.sh

Lines 652 to 661 in 6b9c146

	function run_pass {
	local pass="${1}"
	shift 1
	log_callout -e "\\n'${pass}' started at $(date)"
	if "${pass}_pass" "$@" ; then
	log_success "'${pass}' completed at $(date)"
	else
	log_error "FAIL: '${pass}' failed at $(date)"
	exit 255
	fi

Based on errexit definition, https://www.gnu.org/software/bash/manual/bash.html#The-Set-Builtin

-e
Exit immediately if a pipeline (see Pipelines), which may consist of a single simple command (see Simple Commands), a list (see Lists of Commands), or a compound command (see Compound Commands) returns a non-zero status. The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command’s return status is being inverted with !. If a compound command other than a subshell returns a non-zero status because a command failed while -e was being ignored, the shell does not exit. A trap on ERR, if set, is executed before the shell exits.

The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command’s return status is being inverted with !.

For example,

➜  ~ cat /tmp/testing.sh
#!/usr/bin/env bash

set -euo pipefail

main() {
  command -v xx # should fail
  command -v ls
}

echo $SHELLOPTS
if ! main; then
  echo oops
fi

main
➜  ~
➜  ~ /tmp/testing.sh
braceexpand:errexit:hashall:interactive-comments:nounset:pipefail
/usr/bin/ls
➜  ~
➜  ~ echo $?
1
➜  ~

So we should add || return if the function has multiple calls 😂

EDIT: change ctr to ls.

[ahrtr]

Thanks for the clarification.

The exit 255 at line 660 will still exit the script?

[fuweid]

Yes. exit 255 will exit the process(script)

[ahrtr]

Really good catch!! It works after updating your example like below.

#!/usr/bin/env bash

set -euo pipefail

main() {
  command -v xx || return $? # should fail
  command -v ls
}

#echo $SHELLOPTS
if ! main; then
  echo oops
fi
echo $?

main

[fuweid]

side note: fast fail return if there is any error during build

[fuweid]

side note: go_srcs_in_module doesn't receive any input.

[fuweid]

ping @ahrtr @serathius please take a look. The fail one is tracked by #15545

[serathius]

LGTM, what do you plan about TODOs left? Address in next PR? It's usually better to set TODOs on issues not specific people. This way they can be tracked.

[fuweid]

@serathius Updated with TODO and use #15549 to track it~
It should be fixed in the follow-up and maybe we can provide a fmt_pass for all of lint rules, just like what release-3.4 does.

[serathius]

cc @ptabor @ahrtr

[ahrtr]

Looks great!

Thank you @fuweid

[fuweid]

Thanks for the review. The TODO items will be handled in the follow-up.