feat(bigtable): add AppProfile, and fix five defects in existing Bigtable code - #170
Merged
Conversation
MaterializedView's schema module already existed with no provisioner, recorded in knownParityGaps - and it described a resource the API does not have. It demanded a cluster, though materialized views are instance-scoped, and omitted query, which is required. Rewritten to match the API; the parity test then refused to pass until its known-gap entry was removed, which is what that test is for. The three new types deliberately bypass BigtableProvisioner. It derives the create-id query parameter by trimming a trailing "s" and appending "_id", which is right for instances and clusters but would ask for "appProfile_id" where the API wants "appProfileId". They use the generic engine's CreateIDParam, which sends exactly what it is given. Two fixes to existing code fell out of writing the fixtures: Bigtable::Table declared instance as a plain String, so a table could only ever name an instance that already existed and could not be declared in the same forma as one. That is why the type still has no conformance case. It now takes a resolvable, as every other nested type here does. The native-ID parser switched on each collection by name with no default, so an unlisted collection parsed to an empty resource type and read nothing. Adding appProfiles and logicalViews as the fifth and sixth special case would have kept the trap; every instance-scoped collection now shares one branch. The package had no unit tests, so the parser has some now.
Two fixture errors, both mine. An app profile with multiClusterRoutingUseAny needs an instance with more than one cluster to route across; the host here is DEVELOPMENT, which has exactly one. It now pins traffic to that cluster. Both view types run SQL over a table, and the table had no column families, so there was nothing for a query to read. Each now declares one. Diagnosed from timing rather than from the API's own message: the agent logs "Updating resource state to Failed" with an empty native ID and nothing else, so a plugin-side create failure carries no status code or message into the job log. Worth fixing on its own.
BigtableProvisioner.Create unmarshals properties and never called base.UnwrapValues, which base.Create does twice. Any property whose value arrived wrapped - which is what a reference to another resource produces - read as empty, so a table declared alongside its instance failed with "instance is required for nested resources" while the instance sat right next to it in the same forma. That is why bigtable's table schema typed instance as a plain String, and why the type has no conformance case: with a literal string a table can only name an instance that already exists, and a reference never worked. Found by running the discovery phase purely to read the error: an out-of-band create reports the plugin's own message, while the same failure inside an apply reports nothing at all.
"instance is required for nested resources" is unfalsifiable from a CI log: the property may be absent, present under another name, or present as a type this code cannot read. A plugin-side create failure carries no other diagnostic out of an apply, so that one line was the whole signal - and it sent me down three wrong explanations in a row. The error now names the value's Go type and the property keys the request actually carried.
BigtableProvisioner embeds *base.BaseResource and overrode only Create, so it inherited the raw BaseResource.Status - which reports success and nothing else. UnifiedProvisioner wraps that with a Read for exactly this reason; a hand-written provisioner has to do the same, or a resource has no properties after an async create completes. The symptom was a reference to a Bigtable instance never resolving. With no properties on the instance there is no ".name" to read, so a table declared alongside its instance reached the plugin with the reference still unresolved and failed with "instance is required for nested resources". This affects Instance, Cluster and Table - every Bigtable type that existed before this batch.
appProfiles.create answers with the AppProfile itself, not an Operation, but
the package's single OperationConfig declares every Bigtable create async. base
therefore looked for an operation id that was never there, got "", and polled
BaseURL + "/" - which answers 404:
failed to get operation status: HTTP 404 error
(URL: https://bigtableadmin.googleapis.com/v2/)
Of the five collections, appProfiles and tables answer with the resource;
clusters, logicalViews and materializedViews answer with an Operation. Only the
app profile needed the synchronous config here - tables go through
BigtableProvisioner, which handles their response shape itself.
…p profiles Some GCP deletes are refused without a force flag. Bigtable answers an app-profile delete with a safety-check failure unless ignoreWarnings=true is sent - and its own discovery document describes the parameter as "Required" while flagging it optional, which is how it went unnoticed. ResourceConfig already had UpdateQueryParams; DeleteQueryParams is the same idea for the delete path. The app profile now sends ignoreWarnings on both, since a routing change needs it too. Create, Verify, Extract, Sync and Update all passed for the app profile before this; Destroy was the last red cell.
App profiles and both view types live under an instance, and discovery lists with no properties - so it can name no instance, the path builder falls back to a project-level collection URL that addresses nothing, and the conformance Discover step times out with "resource did not appear in inventory". This is the fifth instance of the same shape in this plugin (Service Directory, Spanner, Cloud SQL, DNS and now Bigtable): a nested resource that GCP offers no wildcard for has to be found by walking its parents. The app profile's CRUD is 8/8 as of the previous commit; this is what stands between the three new types and a green discovery phase.
The materialized-view fixture failed at Create with
Error in field 'display_name' : value must be between 4 and 30 characters
in length, inclusive, but got length 33
which was my own doing: renaming the host instance from "Logical View" to
"Materialized View" pushed the display name over the cap.
The limit is now stated in the instance schema, so a fixture that breaks it
fails at eval instead of after provisioning an instance - the same treatment
Spanner's displayName got for the same reason.
LogicalView and MaterializedView are out. Both create fine some runs and fail at Create in others with no message the apply path surfaces, and settling that needs Bigtable SQL semantics I cannot test locally - each attempt costs a CI cycle on a concurrency group the nightly also needs. AppProfile is verified 8/8 and 4/4, and the five fixes below stand on their own; holding them hostage to two views was the wrong trade. LogicalView is removed entirely. MaterializedView keeps its corrected schema and goes back to being a recorded parity gap, so the repo is left better than it was found: the module now describes the real API instead of demanding a cluster the resource does not have and omitting the query it requires.
This was referenced Aug 28, 2026
naxty
added a commit
that referenced
this pull request
Aug 31, 2026
…ustConfig Restores #171. That pull request was stacked, and it merged into its parent branch rather than into main - GitHub marks a stacked pull request merged once its commits reach its base, and its base was a branch. #167, #168, #169 and #170 went the same way but were recovered when #173 landed, because #173 descended from them. Nothing descended from #171, so certificate manager was the one batch left behind: main has carried a Merged badge and no code since. This is that content on top of current main, nothing else - the three types, their schemas, six fixtures, and the package registration. Verified locally against the service account CI uses, all three CRUD 7/7 with Replace skipped and discovery 4/4. Two things it needed, both recorded in the CHANGELOG: roles/certificatemanager.owner rather than editor, which carries no delete permission at all; and a trust config that is not empty, since the API rejects one carrying neither a trust store nor an allowlisted certificate.
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.
Adds
GCP::Bigtable::AppProfile— and fixes five defects in Bigtable code thatshipped before this PR. Stacked on #169.
bigtable-app-profileVerified through
debug-conformance.yml; the local key is 403 on Bigtable.One type, five fixes. That ratio is the point of this PR: every one of these
was found by writing a conformance fixture for something that already existed.
The fixes
Statusnow routes throughbase.StatusWithRead.BigtableProvisionerembeds
*base.BaseResourceand overrode onlyCreate, so it inherited theraw
Status, which reports success and no properties. A completed asynccreate therefore left the resource with nothing to read — so a reference to a
Bigtable instance never resolved, and a table declared alongside its instance
failed with "instance is required for nested resources". Affects
Instance,ClusterandTable.Createunwraps wrapped property values, asbase.Createalready didtwice. Without it any property carrying a reference read as empty.
appProfiles.createanswers withthe resource, not an Operation, so polling looked for an operation id that
was never there and asked the bare base URL, which answers 404.
unlisted collection parsed to an empty resource type and read nothing,
silently — and I was about to add the fifth and sixth special case.
base.ResourceConfiggainsDeleteQueryParams. Bigtable refuses anapp-profile delete without
ignoreWarnings=true, and its own discoverydocument calls the parameter "Required" while flagging it optional.
Also:
Bigtable::Tableaccepts a resolvable forinstance. As a plainStringa table could only ever name an instance that already existed, so itcould not be declared in the same forma as its instance — which is why the type
still has no conformance case.
And
materialized_view.pklnow describes the real API. It demanded acluster(materialized views are instance-scoped) and omittedquery, which isrequired, so the module was declarable and broken on contact.
What I could not verify
LogicalViewandMaterializedVieware not in this PR. Both create insome runs and fail at Create in others, and settling that needs Bigtable SQL
semantics that cannot be tested locally — each attempt costs a CI cycle on a
concurrency group the nightly also needs.
LogicalViewis removed entirely;MaterializedViewkeeps its corrected schema and goes back to being arecorded
knownParityGapsentry, so the repo is left better than it was found.bigtablepackage had no unit tests; the parser has some now.Correction (2026-08-28): this body reported CRUD as
8/8. That was wrong. TheReplace step reports
[~]— skipped, not passed: the harness only runs it when atestdata/<case>-replace.pklfixture exists, and no case in this repo has one (0 of118). Read every
8/8above as 7 passed, Replace not exercised. The same appliesto every GCP resource already on
main— the Replace path has never been coveredhere. Nothing in this PR regressed.