Add/set status when managing extensions #237
Conversation
ChuckHend
commented
Apr 12, 2023
ChuckHend
commented
- update status when changing or installing new extensions
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| #[serde(default = "defaults::default_extensions")] | ||
| pub extensions: Vec<Extension>, |
There was a problem hiding this comment.
changing the default here to the empty array, rather than making them optional. this puts status in line with how it is defined in spec
| let patch_status = json!({ | ||
| "status": {"running": true} | ||
| }); | ||
| patch_cdb_status_merge(&coredbs, &name, patch_status).await?; | ||
| } |
There was a problem hiding this comment.
I feel like we should always set status.running here too. cc @shahadarsh , maybe we dont need it?
| error!("Error updating CoreDB status: {:?}", e); | ||
| Action::requeue(Duration::from_secs(10)) | ||
| })?; | ||
| patch_cdb_status_force(&coredbs, &name, patch_status).await?; |
There was a problem hiding this comment.
moved this into a function
| pub async fn patch_cdb_status_force( | ||
| cdb: &Api<CoreDB>, | ||
| name: &str, | ||
| patch: serde_json::Value, | ||
| ) -> Result<(), Action> { | ||
| let ps = PatchParams::apply("cntrlr").force(); | ||
| let patch_status = Patch::Apply(patch); | ||
| let _o = cdb.patch_status(name, &ps, &patch_status).await.map_err(|e| { | ||
| error!("Error updating CoreDB status: {:?}", e); | ||
| Action::requeue(Duration::from_secs(10)) | ||
| })?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn patch_cdb_status_merge( | ||
| cdb: &Api<CoreDB>, | ||
| name: &str, | ||
| patch: serde_json::Value, | ||
| ) -> Result<(), Action> { | ||
| let pp = PatchParams::default(); | ||
| let patch_status = Patch::Merge(patch); | ||
| let _o = cdb.patch_status(name, &pp, &patch_status).await.map_err(|e| { | ||
| error!("Error updating CoreDB status: {:?}", e); | ||
| Action::requeue(Duration::from_secs(10)) | ||
| })?; | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
two patch helpers - one for the merge and one for the force
| 'loc: for loc_desired in extension_desired.locations.clone() { | ||
| for loc_actual in extension_actual.locations.clone() { | ||
| if loc_desired.database == loc_actual.database { | ||
| // TODO: when we want to support version changes, this is where we would do it | ||
| if loc_desired.enabled != loc_actual.enabled { | ||
| debug!("desired: {:?}, actual: {:?}", extension_desired, extension_actual); | ||
| changed.push(extension_desired.clone()); | ||
| break 'loc; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
this fixes a bug where we'd continuously reconcile an extension when the description of the extension has changed. We have no way to change the description of an extension, but we do change whether they are enabled. Therefore, only consider it a "change" if the enable toggle has changed, rather than any attribute about the extension.
| "status": {"extensionsUpdating": true} | ||
| }); | ||
| // TODO: we should have better handling/behavior for when we fail to patch the status | ||
| let _ = patch_cdb_status_merge(cdb_api, name, status).await; |
ChuckHend
requested review from
ianstanton,
nhudson and
sjmiller609
as code owners
* Add functionality to reconcile extensions for multiple pods * add just macro to run telemetry with jaeger * adding a few more minor changes * adding name to debug message * rework code, write unit tests * fmt * fix logging
