refactor: extract dereference/validate pipeline from reconcile#889
refactor: extract dereference/validate pipeline from reconcile#889maltesander wants to merge 12 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the unused `namespace` field from `ValidatedInputs` and its corresponding `MissingTrinoNamespace` error variant, along with the `let _ = &validated.namespace` workaround in `reconcile_trino`. Expose `validated_product_config` as `pub(super)` so `test_env_overrides` can call it directly instead of inlining the 30-line equivalent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
adwk67
left a comment
adwk67
left a comment
There was a problem hiding this comment.
There are some inconsistencies with naming, parameter order/type etc.: some should be changed here and some in the other PRs.
| } | ||
|
|
||
| /// Validates the cluster spec and the dereferenced inputs. | ||
| pub fn validate( |
There was a problem hiding this comment.
in Airflow and Hive this is called validate_cluster, so i think we should be consistent.
There was a problem hiding this comment.
In opensearch it is validate?
| type Result<T, E = Error> = std::result::Result<T, E>; | ||
|
|
||
| /// Synchronous inputs the rest of `reconcile_trino` needs after dereferencing. | ||
| pub struct ValidatedInputs { |
There was a problem hiding this comment.
This is cluster-name specific in Airflow and Hive i.e. ValidatedAirflowCluster etc. I'M probably in favour of keeping it generic as it is here.
|
|
||
| /// Synchronous inputs the rest of `reconcile_trino` needs after dereferencing. | ||
| pub struct ValidatedInputs { | ||
| pub resolved_product_image: ResolvedProductImage, |
There was a problem hiding this comment.
just call this variable image to be consistent with the other PRs?
| operator_environment: &OperatorEnvironmentOptions, | ||
| dereferenced: &DereferencedObjects, | ||
| ) -> Result<ValidatedInputs> { | ||
| let resolved_product_image = trino |
There was a problem hiding this comment.
I'll change the Airflow/Hive/Hbase PRs as we have it (wrongly) in dereference and not validate
| }, | ||
| }; | ||
|
|
||
| #[derive(Snafu, Debug, EnumDiscriminants)] |
There was a problem hiding this comment.
Do we need EnumDiscriminants here? Airflow et al don't have it.
| trino: &v1alpha1::TrinoCluster, | ||
| product_config: &ProductConfigManager, | ||
| operator_environment: &OperatorEnvironmentOptions, | ||
| dereferenced: &DereferencedObjects, |
There was a problem hiding this comment.
This order is inconsistent with the others, where we have e.g.
airflow: &v1alpha2::AirflowCluster,
dereferenced: &DereferencedObjects,
product_config_manager: &ProductConfigManager,
(operator_environment: &OperatorEnvironmentOptions will be added to the others when image resolution is moved to validate).
| pub struct ValidatedInputs { | ||
| pub resolved_product_image: ResolvedProductImage, | ||
| pub trino_authentication_config: TrinoAuthenticationConfig, | ||
| pub validated_role_config: ValidatedRoleConfigByPropertyKind, |
There was a problem hiding this comment.
The other PRs have e.g.
pub role_groups: BTreeMap<AirflowRole, BTreeMap<String, ValidatedRoleGroupConfig>>,
pub role_configs: BTreeMap<AirflowRole, ValidatedRoleConfig>,
2 participants
Description
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker