Update status once installation CR is written #668
Conversation
| // A status is needed at this point for operator scorecard tests. | ||
| // status.variant is written later but for some tests the reconciliation | ||
| // does not get to that point. | ||
| instance.Status = operator.InstallationStatus{} |
There was a problem hiding this comment.
This will overwrite any previously written status. Is that what we want here? Or should we be maintaining any status that the controller had previously written?
There was a problem hiding this comment.
No, that wasn't right. I've updated the commit so that we only do the pre-write of the installation status if there was no status at the start of the reconcile.
| // status.variant is written later but for some tests the reconciliation | ||
| // does not get to that point. | ||
| instance.Status = operator.InstallationStatus{} | ||
| if err = r.client.Status().Update(ctx, instance); err != nil { |
There was a problem hiding this comment.
Makes sense during the first run. But on subsequent runthroughs of the reconcilliation loop, won't this nuke existing statuses? Or is that not a problem because we mainly write status to the tigerastatus CR instead of the installation CR?
FWIW, I ran into this same gap while working on a different feature and this is what I came up with: ozdanborne@df37bb4 . Though my code is different in that I'm trying to initialize the tigerastatus, not the installation.status
There was a problem hiding this comment.
What I had was wrong. But thanks for sharing that diff... I've written my fixed commit so that it might merge with your future changes a bit more easily.
There was a problem hiding this comment.
For the most part this looks reasonable. But higher level I'm wondering if there is a 'real' status we should be adding at this point. It just feels like we're adding this to check a box and not providing any real information.
- What status information does the operator scorecard expect to be present?
- A 'real' status that we could include here is a 'Valid' field indicating that the resource has been parsed and looks to be valid.
- Maybe since we intend our main status reporting to be through the tigerastatus resource we should include a reference in the Installation.Status that points to the tigerastatus? (just throwing out ideas)
|
Thanks for looking @tmjd
The test checks whether the Installation CR has a status block present. It's
That's a reasonable idea, maybe we can revisit that later, wdyt?
👍 I think there's something to explore there for sure. But for the sake of satisfying the scorecard test maybe we can just add in the empty status block for now since it's an easier change to approve since we're not changing the API. |
Update status once installation CR is written
Update status once installation CR is written
Update status once installation CR is written
Update status once installation CR is written
Update status once installation CR is written
Update status once installation CR is written
Update status once installation CR is written
Description
This PR writes an empty status block after we've written the installation CR to the datastore. We want this change because an operator scorecard test checks for the existence of the
statusin the CR after applying the CR manifest. The operator scorecard tests are run when we submit an operator metadata bundle to certify a new version of our operator.We'll want to backport this to 1.6, 1.7 and 1.8 to cover Calico v3.14 and v3.15.
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.