chore: Consolidate error enums and use expect where possible

[adwk67]

Description

Part of stackabletech/issues#883

--- PASS: kuttl (812.77s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/smoke_hbase-2.6.6_hdfs-3.5.0_zookeeper-3.9.5_listener-class-external-unstable_openshift-false (800.85s)
        --- PASS: kuttl/harness/smoke_hbase-2.6.6_hdfs-3.5.0_zookeeper-3.9.5_listener-class-cluster-internal_openshift-false (812.76s)
PASS

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.
• Helm chart can be installed and deployed operator works
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible
• Links to generated (nightly) docs added
• Release note snippet added

Reviewer

• Code contains useful comments
• (Integration-)Test cases added
• 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
• Links to generated (nightly) docs added
• Release note snippet added
• Add type/deprecation label & add to the deprecation schedule
• Add type/experimental label & add to the experimental features tracker

[siegfriedweber]

LISTENER_PVC_NAME should be covered by a test_constants() in listener.rs, where it is defined, rather than from the one in statefulset.rs. That also removes the need to make it pub, since its only other use is build_listener_pvc in the same module.

[adwk67]

See e7d3a4f

[siegfriedweber]

This applies to every builder whose build() can fail and where the error handling has been replaced with expect("The annotation keys are static and annotation values cannot be invalid.").

Why a builder fails is generally an implementation detail of the builder. It typically fails either because an input was invalid or because a required method such as metadata was not called. Reading the current implementation and encoding "here is why it cannot fail today" into an expect couples us to that implementation, and that is what makes it risky.

I am raising it on this particular builder because it is less trivial than most. Something like

ConfigMapBuilder::new()
    .metadata(some_metadata)
    .add_data(...)
    .build()
    .expect("metadata is set")

has the same problem in principle, but with a bit of good will it is acceptable.

This builder has many method calls, and it is not obvious which input ends up in the annotations the message talks about. It currently accepts any string as the service scope, and this function forwards whatever it is given. If the builder were extended to check that the string is a valid Service name, the operator would crash with the message "the annotations are valid". Granted, the example is contrived, since the service name used here is defined statically, but the operator can change in ways we cannot foresee.

I would do one of the following:

• keep the error handling on the less trivial builders, or
• change the message to something like "all inputs are valid and complete, so the builder does not fail", and commit to
• making the builders infallible.

Making builders infallible is hard in general, but for the ones in stackable-operator it should be tractable: required fields could be set in the constructor, methods could accept only
validated types, and coupled methods such as with_format(TlsPkcs12) and with_tls_pkcs12_password could be merged into one.

[adwk67]

Expects have been reverted other than when the inputs are static: this keeps things more readable and any builder input that depends on code elsewhere is safer with a Result.