Simple tooling to generate CRDs for LoggingLogMetric #1754
Conversation
justinsb
commented
May 9, 2024
•
justinsb
commented
- LoggingLogMetric: sync up schema labels & printColumns
- LoggingLogMetric: make spec required
- fix: deepcopy-gen annotations on shared package
- chore: create simple script to generate crds from direct apis
- autogen: generate crds using dev/tasks/generate-crds
justinsb
force-pushed
the
sync_crds
branch
2 times, most recently
from
e7fb511
to
f7dfc7c
Compare
| @@ -71,7 +76,6 @@ spec: | |||
| bounds: | |||
| description: The values must be monotonically increasing. | |||
| items: | |||
| format: double | |||
There was a problem hiding this comment.
IIRC, we were told this info is not exactly sent over to the api server but 🙈 we still want these, right?
There was a problem hiding this comment.
Yes, we believe this is not actually used. That we specify double (float64) but the official tools drop it supports this. So I don't think we need it.
Maybe we should try to figure out how to add it just so we don't need to worry about it, but it would be a pain I think...
There was a problem hiding this comment.
Actually, scratch that, just found the // +kubebuilder:validation:Format directive
There was a problem hiding this comment.
Although this particular one is a problem because the format has moved to the slice, which is likely wrong
There was a problem hiding this comment.
+1. I like the approach of using kubebuilder to validate the CRD. Also, we shall have// +kubebuilder:validation:Required on spec.filter.
One thing I don't figure out how it works is that the LLM CRD has filter set as required. But I was assuming the CRD is generated from this file which should have filter as optional (default). Thoughts?
| go run ./scripts/crd-tools set-annotation cnrm.cloud.google.com/version=0.0.0-dev --dir apis/config/crd/ | ||
| go run ./scripts/crd-tools delete-annotation controller-gen.kubebuilder.io/version --dir apis/config/crd/ |
There was a problem hiding this comment.
just curious, any of this can be achieved by kustomize?
There was a problem hiding this comment.
Maybe, but I think we'd have to use KRM functions (e.g. delete-annotation). I figure we can start this way and then hopefully create KRM functions.
Matching the existing schema.
Another small delta with the existing schema.
These simply didn't work when we tried to generate from them.
justinsb
changed the title
We're not sure that these matter, but it means we don't have to worry about it every time.
justinsb
force-pushed
the
sync_crds
branch
2 times, most recently
from
a855c87
to
a8f6e52
Compare
| @@ -162,7 +162,7 @@ type LoggingLogMetricSpec struct { | |||
| bucket has to be in the same project as the metric. For | |||
| example:projects/my-project/locations/global/buckets/my-bucket | |||
| If empty, then the Log Metric is considered a non-Bucket Log Metric. | |||
| Only `external` field is supported to configure the reference. */ | |||
| Only `external` field is supported to configure the reference for now. */ | |||
There was a problem hiding this comment.
Do we need this change? I think the CRD is generated from the ./apis.
There was a problem hiding this comment.
Actually, can we delete the TF-based APIs? Otherwise it could be really easy to get confused.
justinsb
force-pushed
the
sync_crds
branch
2 times, most recently
from
a11fb6f
to
b1d810f
Compare
This makes it easier to understand why things are changing.
We shouldn't need it, and if we do need it, we should set it in the script, for reproducibility.
|
/lgtm /hold (Feel free to cancel the hold, just in case you are on some more commits) |
|
[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 |
|
I split out the last commit as #1843 but we can just merge this if you want |
|
OK, #1843 merged, so let's see if github can figure out this rebase /hold cancel |
google-oss-prow
bot
merged commit 07498c7
into
GoogleCloudPlatform:master
