Refactor/improve testability #243
Conversation
# Conflicts: # rust/operator-binary/src/airflow_controller.rs
There was a problem hiding this comment.
I like this paradigm! Just a general comment: I would prefer to have conditional logic in just one place and not in both fetch and build (my preference would be to have it as "early" as possible, so in fetch or even in reconcile) to make the code a little easier to follow e.g. in the original code the patch for e.g. the airflow_db job was applied in each reconcile - delegating that to k8s to see if there had been any change or not - rather than checking in the code to see if the job exists.
|
|
||
| let (rbac_sa, _) = rbac::build_rbac_resources(airflow_db.as_ref(), "airflow"); | ||
|
|
||
| built_cluster_resources.push(BuiltClusterResource::PatchRBAC); |
There was a problem hiding this comment.
if we note the output of build_rbac_resources (in the previous line) with the PatchRBAC resource type we don't the need the second callout to build_rbac_resources in apply.rs (same applies to airflow_controller).
There was a problem hiding this comment.
Yeah, we could. I was okay with calling the function twice, because it is pure and computes quickly.
| init_job: ObjectRef::<Job>::new(&job_name).within(&ns), | ||
| }); | ||
|
|
||
| let job = match job_result { |
There was a problem hiding this comment.
I think this checking if the job exists could be avoided if the case check against airflow_db.status is moved into the reconcile function: then the status is known and the job can be created explicitly (same applies to airflow_controller). I'm not sure but I don't think this would affect the fetch-build-apply template.
There was a problem hiding this comment.
I could imagine transforming the FetchedAdditionalData into an enum. If the job is missing, the only thing we care about is creating the job. If it exists, all other fields start to matter.
This is still duplicated logic. I'm not sure we can avoid 'knowing' about what's going on in the build step entirely here.
|
Good points Andrew, thanks for taking a look! I see what you mean. I like keeping PRs as focused as possible, so I opted towards making this one about getting the big restructuring done. I think we can improve the clarity of the code (although I must admit, I don't have a clear idea what the rough edges are at the moment). I would have opted for a follow-up PR, but if it is important to be certain that we can put in more work in this PR. My biggest wish is to try this structure with a real-life issue, and see how it feels like to write useful tests while developing new features for the operator as soon as possible. |
|
|
Description
Fixes #242
Splitting up the two controllers within this operator into fetch/build/apply each.
For anyone who would like to review this heap of changes: you will have a better time going through each commit. Also, I'm sorry.
There is a version bump of the dev dependency
serde_yamlhappening in the first part of the PR, which made it necessary to adjust existing ldap unit tests. The bump went under in a bigger commit and is easy to miss.Review Checklist
Once the review is done, comment
bors r+(orbors merge) to merge. Further information