demo: add types for 10 greenfield resources using bulk generation flags - #13678
Draft
ldanielmadariaga wants to merge 20 commits into
Draft
ldanielmadariaga wants to merge 20 commits into
ldanielmadariaga wants to merge 20 commits into
Conversation
TestMissingRefs used to skip every [refs] finding of a resource while it had any queue entry, including placement entries and the untriaged-bulk-generation marker. It now skips a finding only while an open entry with a reference reason names the same kind, group and field. Other fields of the resource are checked as usual. This relies on --emit-reference-hints running with --prepopulate-spec. The hints use refs.Classify, the same rule as the test, so every field the test flags already has an entry at the same path. The reference reasons are now constants in pkg/judgement, used by the generator and by the test, so the two cannot drift. untriaged-bulk-generation stays as a record that nobody has reviewed the resource; it no longer suppresses anything.
…o its proto and docs With the flag, generate-types writes an "API sources" block between the license and the package clause of a new <kind>_types.go: +kcc:source:proto the proto file at the pinned googleapis commit +kcc:source:service-docs the Discovery documentationLink +kcc:source:resource-docs the resource's REST reference page The docs links are guessed and checked online in the same run. A link that does not check out is still written, gets a +kcc:guess=source-link line above it, and an open verify-resource-docs-link or verify-service-docs-link entry in the judgement queue. Network failures never stop generation. Only a newly scaffolded types file gets links, so existing resources do not change.
A +kcc:guess=source-link line with no open entry means the entry was resolved but the link was not fixed. An open entry with no guess line means the link was fixed but the entry was not resolved. Both fail the test.
…hen one is used The derived roots, from the service config and the Discovery directory, are tried first. An override is only reached when none of them verifies, and using one is logged, so we can see which overrides are still needed and remove the rest.
Generate ChronicleWatchlist using the controllerbuilder bulk-generation flags. This resource demonstrates: - Pre-populated spec from proto definition - Required field annotations inferred from proto field behavior - Output-only detection - Judgement call queue integration for parent-ref-not-modelled
Regenerated with the YAML queue from GoogleCloudPlatform#13530 and GoogleCloudPlatform#13531. The three entries are unchanged and all still open. None has a reference reason, so TestMissingRefs checks every ChronicleWatchlist field, and it passes.
unit-tests-operator renders the cnrm-admin and cnrm-viewer ClusterRoles and compares them with this file. The ChronicleWatchlist commit added the chronicle group to both roles but not to this file, so the test failed on this PR.
…ation flags New service, so it uses the full Chronicle flag set plus --emit-source-links. The ScanConfig v1 proto has no google.api.resource annotation. The generator falls back to a project and location parent and queues location-parent-unknown and verify-resource-docs-link. Nothing landed in status.observedState (queued as empty-observedstate). managed_scan ends its comment with "output only", which --detect-output-only-in-comments does not catch: it only matches comments that start with "Output only". Test exceptions: the two password fields (TestNoSensitiveField), the empty observedState (TestCRDObjectTypes), and the alpha missing-fields list. The scaffold templates still write 2025 in the license header; the new files use 2026.
New service, so it uses the full Chronicle flag set plus --emit-source-links. All three source links were verified. network stays a string. It is queued twice: possible-reference (the field holds a network resource name) and possible-reference-by-description. Test exceptions: the alpha missing-fields list only.
The service already has ConfigDeploymentGroup. Adding the full flag set plus --emit-source-links to its generate-types call left the ConfigDeploymentGroup types and CRD unchanged. generate.sh also runs generate-mapper. It now gets --emit-plural-acronyms and --emit-message-maps too, so the mapper names fields the way generate-types does and writes the loops for the three map fields. The existing mapper functions are unchanged. configdeployment_mapper.go is handwritten. It holds the helpers generate-mapper calls but cannot write yet: the proto3 optional enum ProviderConfig.source_type, and google.protobuf.Value. google.protobuf.Value is generated as a Value struct with a ListValue, so the package now has a recursive type (added to recursivetypes.txt, like the three packages already listed there). The proto link points at the KCC copy of config.proto, which generate-proto.sh uses instead of the googleapis file.
GKEBackupRestoreChannel is generated with the full set of bulk generation flags. generate-mapper gets --emit-plural-acronyms and --emit-message-maps so the mapper matches the types. The types of the other five GKEBackup kinds did not change, and no existing mapper function changed. All three source links were verified. .spec.destinationProject stays a string and is queued as a possible reference. No handwritten code was needed. The only test exception is in alpha-missingfields.txt: the new fields have no test yet.
…n flags StorageInsightsReportConfig is generated with the full set of bulk generation flags. generate-mapper gets --emit-plural-acronyms and --emit-message-maps so the mapper matches the types. The types of StorageInsightsDatasetConfig did not change, and no existing mapper function changed. storageinsightsreportconfig_mapper.go is handwritten. generate-mapper cannot map google.type.Date, so the file has Date_FromProto and Date_ToProto, copied from the gkebackup controller. The resource-docs link is wrong and is marked as a guess. The resolver tried the Cloud Storage REST root (storage/docs/reference/rest/v1/projects.locations.reportConfigs), which redirects to an overview page. The real page is under storage/docs/insights/reference/rest. The two bucket fields, objectMetadataReportOptions.storageFilters.bucket and objectMetadataReportOptions.storageDestinationOptions.bucket, stay strings and are not queued. Their comments do not say "Cloud Storage" or "gs://", which the bucket rule needs. Test exceptions: - TestCRDObjectTypes: spec.parquetOptions is an empty object. - alpha-missingfields.txt: the new fields have no test yet.
MigrationCenterSource is generated with the full set of bulk generation flags. generate-mapper gets --emit-plural-acronyms and --emit-message-maps so the mapper matches the types. The types of MigrationCenterGroup and MigrationCenterPreferenceSet did not change, and no existing mapper function changed. All three source links were verified. No handwritten code was needed. The only test exception is in alpha-missingfields.txt: the new fields have no test yet.
SaaSServiceMgmtTenant is generated with the bulk generation flags, except --emit-required-from-proto. That flag would mark UnitVariable.variable as required, which changes the existing SaasServiceMgmtRelease CRD. The flags apply to every resource in the generate-types call, so the new kind loses it too: .spec.saas says "Required" but has no +required marker. generate-mapper gets --emit-plural-acronyms and --emit-message-maps so the mapper matches the types. The types of SaasServiceMgmtRelease did not change, and no existing mapper function changed. All three source links were verified. .spec.saas stays a string and is queued as a possible reference to Saas. No handwritten code was needed. The only test exception is in alpha-missingfields.txt: the new fields have no test yet.
…ation flags ContentWarehouseDocumentSchema is generated with the full set of bulk generation flags. generate-mapper gets --emit-plural-acronyms and --emit-message-maps so it matches the types when it is run again. The types of ContentWarehouseRuleSet, ContentWarehouseDocument and ContentWarehouseSynonymSet did not change. types.generated.go loses 5 lines, all inside commented-out blocks for types no CRD uses. mapper.generated.go is left as it is on master, because the generate-mapper output does not compile. Both causes predate this change: - The file on master is stale. Running the unchanged generate.sh adds 24 functions for the RuleSet and Document types, and they call a Policy_FromProto that does not exist. dev/tasks/generate-types-and-mappers already skips this service. - The pinned googleapis has two PropertyDefinition fields, retrieval_importance and schema_sources, that the genproto package this service uses does not have. All three source links were verified. Test exceptions: - TestCRDObjectTypes: six of the *TypeOptions fields under spec.propertyDefinitions[] are empty objects. - recursivetypes.txt: PropertyDefinition -> PropertyTypeOptions -> PropertyDefinition. - alpha-missingfields.txt: the new fields have no test yet.
The first run of generate-types for a new resource writes a copy of the resource's proto message (and its ObservedState) into types.generated.go, inside a comment. The next run drops it, because the new <kind>_types.go now claims that message. Services that pass --include-skipped-output keep the copy, under a different header. Later runs, and validate-generated-files in CI, produce the second-run output, so this commits it. Only comments in types.generated.go change. A third run of WebSecurityScanner made no further change.
SaaSServiceMgmtRollout replaces LiveStreamChannel, whose Step 1 PR merged while this branch was being built. It joins the generate-types call that already has the bulk generation flags from SaaSServiceMgmtTenant, so it gets the same flags: the full set except --emit-required-from-proto. The types of SaasServiceMgmtRelease and SaaSServiceMgmtTenant did not change, and no existing mapper function changed. The output is from the second run of generate-types, which is what later runs produce. All three source links were verified. .spec.release and .spec.rolloutKind stay strings and are queued as possible references. No handwritten code was needed. The only test exception is in alpha-missingfields.txt: the new fields have no test yet.
AIPlatformCachedContent replaces FinancialServicesInstance, whose Step 1 PR merged while this branch was being built. AIPlatformModelMonitor, the other alternate, was tried first and does not compile: ModelMonitor is only in v1beta1, and adding it gives 19 duplicate Go type names, such as ExplanationSpec and MachineSpec. The existing kinds generate those types from v1. aiplatform lists both google.cloud.aiplatform.v1 and v1beta1. Three flags are left out because they change the types of existing kinds: --emit-required-from-proto, --emit-plural-acronyms and --emit-sibling-refs. The generate.sh comment says what each one changes. The generate-mapper call is unchanged: the new types have no renamed fields and no message maps. The types of the 10 existing kinds did not change. The output is from the second run of generate-types, which is what later runs produce. gofmt reformats one commented-out block in mapper.generated.go, because a new function now follows it. Handwritten code: - aiplatformcachedcontent_mapper.go: helpers for google.type.LatLng and CachedContent.UsageMetadata. mapper.generated.go calls them, but generate-mapper does not write them. - mapper.go: the handwritten Schema mapper now uses the v1 Schema instead of v1beta1. The new v1 FunctionDeclaration mapper calls it, and nothing else did. The two messages have the same fields, so only the import changes. The bot PR (GoogleCloudPlatform#12696) made the same change. Test exceptions: an allowlist entry for empty objects (tools[].codeExecution, googleMaps and urlContext have no fields), one acronyms.txt entry (ragFileIds), and alpha-missingfields.txt. The queue also gets an ambiguous-proto-name entry for each of the 10 existing kinds, because their messages exist in both versions. The resource docs link is a guess, and it is wrong: the page is under vertex-ai/generative-ai/docs/reference/rest.
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…ccepts the CRD Value contains ListValue, so the type is recursive. controller-gen left the items of listValue.values as an empty schema, and envtest could not install the ConfigDeployment CRD: items.type: Required value: must not be empty for specified array items ListValue is now written by hand in configdeployment_types.go, with items:Type=object and items:XPreserveUnknownFields on the list, as DataplexAspectType does. generate-types no longer generates it.
…I server accepts the CRD PropertyDefinition contains PropertyTypeOptions, so the type is recursive. controller-gen left the items of propertyTypeOptions.propertyDefinitions as an empty schema, and envtest could not install the ContentWarehouseDocumentSchema CRD. PropertyTypeOptions is now written by hand in contentwarehousedocumentschema_types.go, with the same two markers as ConfigDeployment's ListValue. generate-types now leaves a commented-out copy in types.generated.go, as it does for the other hand-written types in this package.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #13407 (Chronicle), which is stacked on #13556 and #13531. The last 13 commits are new here.
Draft. Not meant to merge as is. This runs the bulk generation flags from the earlier PRs on 10 resources whose greenfield Step 1 (types) is still open, to show how far they get. All 10 have an open bot PR for the same Kind, so the output can be compared.
Tracking issue: #13411
How the 10 were picked
Issues labelled
cp/leafandworkflow/greenfieldwhose Step 1 types PR is not merged. I spread them across services: 2 new service folders, and 8 resources in 7 services that already have resources. Kind names follow the bot PRs, for exampleSaaSServiceMgmtTenant, though the group's existing Kind isSaasServiceMgmtRelease.Two bot PRs merged while I was working: LiveStreamChannel (#13648) and FinancialServicesInstance (#13642). I dropped both from this branch and used SaaSServiceMgmtRollout and AIPlatformCachedContent instead. Before that, they were a useful check: each merged version matched ours except for references (3 in LiveStreamChannel,
kmsKeyin FinancialServicesInstance), and our queue had flagged all of them. For FinancialServicesInstance, ourtypes.generated.gowas byte-identical to the merged one. AIPlatformModelMonitor was tried before AIPlatformCachedContent and does not compile (finding 11).How each one was generated
generate.shcopied from Chronicle's, with the full flag set. No mapper, as in Chronicle.--resourceand the flags go into the existinggenerate-typescall, with a comment.generate-mappergets--emit-plural-acronyms --emit-message-mapsso the mapper matches the types (not in aiplatform, see below)._types.gofiles are byte-identical;types.generated.goandzz_generated.deepcopy.goonly gain code (contentwarehouse loses 5 lines, all inside commented-out blocks; in aiplatform, the one import line becomes an import block);Schemamapper in aiplatform (see the table).--emit-required-from-proto. It would markUnitVariable.variableas required, which changes theSaasServiceMgmtReleaseCRD.--emit-required-from-proto,--emit-plural-acronymsand--emit-sibling-refs. They would add+requiredto existing structs, renameGCSSource.UristoURIs, and add reference guesses to two existing status structs. Itsgenerate-mappercall is unchanged.Full flag set:
--prepopulate-spec --emit-required-from-proto --emit-plural-acronyms --emit-message-maps --place-server-set-fields --detect-output-only-in-comments --emit-parent-refs --emit-sibling-refs --emit-reference-hints --emit-source-linksResults
sensitive.txt(2 passwords); emptystatus.observedStategoogle.protobuf.Valueand a proto3 optional enum;ListValuetype (finding 7)recursivetypes.txt(Value)google.type.DateparquetOptions)PropertyTypeOptionstype (finding 7); mapper left as on master (finding 9)recursivetypes.txtgoogle.type.LatLngandCachedContent.UsageMetadata; theSchemamapper now imports v1 instead of v1beta1 (one line)acronyms.txt(ragFileIds); empty objects¹ The proto comes from
mockgcp/apis, so the link points at this repo'sblob/master, which is not pinned.untriaged-bulk-generationand one aboutlocation. The rest are mostly possible references.alpha-missingfields.txtentries until it has fixtures.Compared with the bot PRs
A script compared the field paths of our CRD and the bot's CRD for each of the 10.
network, ConfigDeploymentserviceAccountandworkerPool, GKEBackupdestinationProject, SaaS Tenantsaas, SaaS RolloutreleaseandrolloutKind. The bot wrote a*Reffield. We left a string and queued the field as a possible reference.storageFilters.bucketandstorageDestinationOptions.bucket. The bot wrotebucketRef. We left strings and queued nothing.sensitive.txt;managedScanto status; ours stayed in spec;location; ours has it, with alocation-parent-unknownqueue entry, because the proto declares no resource pattern.terraformBlueprint.inputValues,terraformBlueprint.externalValuesanddeleteResults.outputs. They are maps of messages, which--emit-message-mapskeeps and the bot dropped.labelsandannotations. The bot dropped them.any-typed stubs for the twoSchemaSourcefunctions so thatgenerate-mapperskips them.missingrefs.txt. Our queue marks the same five as possible references, plusencryptionSpec.kmsKeyName. The bot has the sameacronyms.txtentry and the same 155alpha-missingfields.txtentries. Itsmapper.gohas the same three changes:LatLngandUsageMetadatahelpers, and theSchemamapper switched to v1.Findings
None of these are fixed here.
generate-typesrun writes a commented-out copy of the resource's message intotypes.generated.go. The second run drops it, because the new_types.gonow claims that message. CI'svalidate-generated-fileschecks the second-run output, so a new resource needs two runs. The "Rerun generate-types" commit is that second run.bucketrule needs "Cloud Storage" orgs://in the comment. StorageInsights'bucketfields say neither, so both were missed;networkand ConfigDeploymentserviceAccountandworkerPoolgetpossible-referenceandpossible-reference-by-description; SaaS Rolloutreleasegets three entries, one of them left over from the first run.storage/docs/insights/reference/rest. It is marked as a guess, so the check did its job;vertex-ai/docs/reference/rest; the page is undervertex-ai/generative-ai/docs/reference/rest. It is marked as a guess;livestream_v1.yamlhas no docs URL. An empty+kcc:sourceline isn't useful; it should probably be left out.managed_scancomment ends with "output only", so the field stayed in spec.TestNoSensitiveFieldcatches them; the queue doesn't.--emit-required-from-protoout for SaaS also drops+requiredfromTenant.saas, whose comment says "Required". aiplatform leaves out three flags for the same reason.google.protobuf.Value(ConfigDeployment) andPropertyDefinition(ContentWarehouse) contain themselves through a list. controller-gen leaves that list'sitemswithout atype. apichecks passed, but envtest could not install the CRDs, so the first CI run failed. The fix is two markers on the list field,items:Type=objectanditems:XPreserveUnknownFields, as inDataplexAspectType. To add them, I movedListValueandPropertyTypeOptionsinto the_types.gofiles. The bot made the same fix for ContentWarehouse, by editingtypes.generated.go. The generator could add these markers itself, or mapgoogle.protobuf.Valueto JSON. Filed as Generated types: recursive list fields give CRDs the API server rejects (items without type) #13680.google.type.Date,google.protobuf.Value,google.type.LatLng, a proto3 optional enum,google.type.TimeZone(LiveStreamChannel, dropped), andCachedContent.UsageMetadata. That last one is a message nested in the resource message:generate-mapperwrites the ObservedState mapper that calls it, but not the function itself.generate-mapperoutput doesn't compile, for two reasons that predate this PR, somapper.generated.gois left as on master:generate.shadds 24 functions for RuleSet and Document that call a missingPolicy_FromProto.dev/tasks/generate-types-and-mappersalready skips this service;PropertyDefinitionfields that the genproto package used here doesn't have.message-map-derivedentry forMapProperty.fields, a type no CRD uses.google.cloud.aiplatform.v1andv1beta1:ExplanationSpecandMachineSpec. The existing kinds generate those types from v1. The bot PR (Greenfield: Implement direct KRM types, identity, and generate.sh for AIPlatformModelMonitor #12856) rewrote proto names withsedingenerate.shand changed the type generator;Schemamapper took the v1beta1 message, but the new v1FunctionDeclarationneeds v1. Nothing else called it, and the two messages have the same fields, so only its import changed;ambiguous-proto-nameentry, because its message is in both versions. The warning suggests putting the full proto name in--resource.Verification
go test ./tests/apichecks/after each commit.make fmt,go vet ./...,go test ./...indev/tools/controllerbuilder, andgo test ./pkg/fuzztesting/fuzztests/.go test ./cmd/recorder -run TestProfileRecorderFootprint, which installs every CRD with envtest.