chore: Consolidate error enums and use expect where possible - #869
Conversation
siegfriedweber
left a comment
There was a problem hiding this comment.
In the airflow-operator the expects sit at the call site (pb.add_volumes(…).expect(…)), so the constraint is visible exactly where it has to be honoured: you can see at a glance that nothing may be expected after the user-supplied volumes and volume mounts have been added.
Here most of the adding is delegated to helpers instead: add_tls_volume_and_volume_mounts, add_config_volume_and_volume_mounts, add_log_config_volume_and_volume_mounts, add_log_volume_and_volume_mounts, add_hdfs_cm_volume_and_volume_mounts and RoleResource::update_volumes_and_volume_mounts. From the call site there is no sign that any of them can panic, nor that they must run before the user-supplied additions. The ordering requirement is real, but invisible to anyone editing build_rolegroup_statefulset.
Two ways to address that:
- Document it on the helpers, e.g.
/// # Panics
///
/// Panics if the volumes or volume mounts cannot be added to the builders. Only call this
/// on builders whose volume names and mount paths are still distinct from the ones added
/// here.
- Keep the helpers returning
Resultand move theexpects to the call site, as in the airflow-operator.
|
I opted for the |
Description
Part of stackabletech/issues#883
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker