fix: wait_for_termination returns actual exit status

[sweb]

Description

since set -e is the last part of wait_for_termination it always has exit code 0. With this, it returns the exit code of the process it is waiting for instead. [ "${term_child_pid}" ] blows up when the child pid is not set yet, so gets changed to [ -n "${term_child_pid:-}" ].

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added

[lfrancke]

Ah...we just had the same/similar in stackabletech/stackable-utils#130

It's an annoying trap.

[sweb]

Ah...we just had the same/similar in stackabletech/stackable-utils#130

It's an annoying trap.

in this context, it is not a huge deal - from what I checked it does not interfere with OOM based restarts or something like that - but getting the actual exit codes may be nice and should be an improvement.

[maltesander]

A couple of things. Almost all downstream products require changes:

• https://github.com/stackabletech/trino-operator/blob/77bcf6da54ac49fb2d896388f791611f79984a50/rust/operator-binary/src/controller/build/command.rs#L145
• https://github.com/stackabletech/nifi-operator/blob/4d6cea98741fb34c29d28ae4e13931dcb317d817/rust/operator-binary/src/controller/build/resource/statefulset.rs#L404

Will abort under set -e with non-zero code, neither create_vector_shutdown_file_command or other lines after that will be executed (e.g. the vector shutdown file etc.). So || product_exit_code=$? should be added and rolled out with an operator-rs release properly?

And i think a similar problem for git-sync containers

operator-rs/crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

Line 355 in 4e5b186

wait_for_termination $!"

---

It could make sense to stop that test script duplication (i think its in over 10 places) and e.g. use insta (already in the repo) as dev/test dependency? Something like:

insta::assert_snapshot!(
    serde_yaml::to_string(&git_sync_resources.git_sync_containers.first()).unwrap()
);

[maltesander]

Can we add a flag / comment for the 128?

[sweb]

addressed here: f3a4fee

still looking into insta.