tests: Don't ignore annotations with direct reconciler #1919
Conversation
justinsb
force-pushed
the
dont_hide_state_into_spec
branch
2 times, most recently
from
7ae436e
to
3367cd1
Compare
pkg/test/utils.go
Outdated
| } | ||
| } | ||
|
|
||
| func postWriteNormalization(wantMap map[string]interface{}) map[string]interface{} { | ||
| if os.Getenv("KCC_USE_DIRECT_RECONCILERS") != "" { | ||
| if os.Getenv("XXX_KCC_USE_DIRECT_RECONCILERS") != "" { |
There was a problem hiding this comment.
Yes, it's a quick hack while this is WIP!
There was a problem hiding this comment.
That said it was confusing, so I just deleted it instead of hacking-it-out
justinsb
force-pushed
the
dont_hide_state_into_spec
branch
2 times, most recently
from
e7b81cd
to
3c320cd
Compare
|
Yes I think this should work well now that the LoggingLogMetric resource is a direct controller based one. For resource conversions some normalization might still be needed. |
We should update the golden data, rather than ignoring it.
Update the scenario data to match the transition of LoggingLogMetric to direct actuation.
justinsb
force-pushed
the
dont_hide_state_into_spec
branch
from
3c320cd
to
a93f167
Compare
justinsb
changed the title
|
/lgtm Thank you! |
| } | ||
| } | ||
|
|
||
| func postWriteNormalization(wantMap map[string]interface{}) map[string]interface{} { |
There was a problem hiding this comment.
+1 to not override the test expectations.
What is the current LLM behavior if user specify those two annotations? Is it a no-op or we have some handling in the direct controller(I don't recall I see any, just to double check in case I miss anything)?
I recall in SIS, we decide to add some warnings if user specify the unexpected annotations.
There was a problem hiding this comment.
It's a no-op, I believe, but @acpana can confirm
| @@ -24,24 +24,23 @@ cd ${REPO_ROOT}/ | |||
| echo "Downloading envtest assets..." | |||
| export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path) | |||
|
|
|||
| #export KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric | |||
| echo "Running e2e tests samples for LoggingLogMetric direct reconciliation..." | ||
|
|
||
| KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \ |
There was a problem hiding this comment.
ah! you bit me. I fix this in my WIP PR.
There was a problem hiding this comment.
Happy to merge yours first if you prefer!
| @@ -255,74 +171,18 @@ X-Xss-Protection: 0 | |||
| "labels": [ | |||
| { | |||
| "description": "a label description", | |||
| "key": "testkey" | |||
There was a problem hiding this comment.
Is the previous log from the real GCP service? From the deleted part, I think we want to modify the mock llm to use operation metadata to reflect the real http as much as possible. May be a good follow-up task for @acpana ?
There was a problem hiding this comment.
I think the mock for LoggingLogMetric is pretty good, but agree we should ensure that we do have a recording with real GCP. This is currently recorded with the mock
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit eae1fe8
into
GoogleCloudPlatform:master
We should update the golden data, rather than ignoring it.