Create webhook for VerticaRestorePointQuery CR #709
Conversation
chinhtranvan
force-pushed
the
chinh/create--webhook-for-VerticaRestorePointQuery-CR
branch
from
05948ab
to
fbf8a86
Compare
There was a problem hiding this comment.
Thanks for adding this in. A few minor comments in addition to what I already added:
- There should be an update to PROJECT to indicate that we have a webhook now for the CR.
- Unrelated to this PR, but I noticed it while playing with vrpq. Can we remove
omitemptyfrom VerticaRestorePointsQueryStatus.RestorePoints? I created a vrpq that didn't return anything and that field wasn't there. I think it will be clear if we have it even if it is empty. That is what most people are going to look for when they create the CR.
There was a problem hiding this comment.
I'm a little confused about this rename. Are you sure this is suppose to be an assert file? It looks like we have logic in it to create a vrpq. It should only be checking what was done (or not done) in 20-invalid-startTimestamp.yaml.
There was a problem hiding this comment.
As discussed in the scrum, we should add one more step filter for vrpq
| @@ -15,5 +15,5 @@ apiVersion: kuttl.dev/v1beta1 | |||
| kind: TestStep | |||
| commands: | |||
| - script: |- | |||
| export STARTTIMESTAMP=$(kubectl get vrpq archive-query -o json -n $NAMESPACE | jq -r '.status.restorePoints[] | select(.index==1 and .archive=="db") | .timestamp') | |||
| export STARTTIMESTAMP="$(kubectl get vrpq archive-query -o json -n $NAMESPACE | jq -r '.status.restorePoints[] | select(.index==1 and .archive=="db") | .timestamp')" | |||
There was a problem hiding this comment.
@spilchen There is one thing I've noticed: when I run it without double quotes on my local machine, it doesn't work. It freezes, possibly due to the timestamp containing a space. I'm unsure why it passes in the GitHub CI but not on my local machine. After discussing with @jizhuoyu , we concluded that adding double quotes would make it more persistent and reliable
|
Can we remove omitempty from VerticaRestorePointsQueryStatus.RestorePoints? I think that will make it clear when we create the CR and no results are returned. |
Never mind, I noticed that it's on the server side. |
yes, I already opened a PR on the server side, you can take a look it https://git.verticacorp.com/projects/VER/repos/server/pull-requests/20274/overview |
chinhtranvan
force-pushed
the
chinh/create--webhook-for-VerticaRestorePointQuery-CR
branch
3 times, most recently
from
816800c
to
df3f964
Compare
| @@ -146,7 +147,7 @@ func (q *QueryReconciler) runShowRestorePoints(ctx context.Context, dispatcher v | |||
| opts []showrestorepoints.Option) (err error) { | |||
| // set Querying status condition ,state and restore points prior to calling vclusterops API | |||
| err = vrpqstatus.Update(ctx, q.VRec.Client, q.VRec.Log, q.Vrpq, | |||
| []*metav1.Condition{vapi.MakeCondition(v1beta1.Querying, metav1.ConditionTrue, "Started")}, stateQuerying, nil) | |||
There was a problem hiding this comment.
I'm guessing this change is needed because the restorePoints list is required now? Did you try setting the optional marker?
// +kubebuilder:validation:Optional
There was a problem hiding this comment.
Oops, why didn't I think this setting? Yes, this setting is better. We don't need to update nil to vops.RestorePoints{}, and it still returns an empty status for Restore Points in the CR. Will fix it
chinhtranvan
force-pushed
the
chinh/create--webhook-for-VerticaRestorePointQuery-CR
branch
from
df3f964
to
9d1e885
Compare
chinhtranvan
deleted the
chinh/create--webhook-for-VerticaRestorePointQuery-CR
branch
This PR creates a webhook for the new VerticaRestorePointQuery CR to catch the checks early. There is some webhook part that is added in #643."