Greenfield: Implement direct controller, E2E fixtures, and fuzzer for MapManagementMapConfig - #12428
Conversation
ada-coder-bot
added
overseer
greenfield
Audit Logs from Real GCP TestingHere are the audit logs retrieved showing that the MapManagement API service was successfully enabled and exercised for the tests: ---
insertId: pkx2j1dq6o8
logName: projects/cnrm-barni-4/logs/cloudaudit.googleapis.com%2Factivity
operation:
id: operations/acat.p2-600845353393-46f1fb40-5b83-4e84-bb03-d932099f8e69
last: true
producer: serviceusage.googleapis.com
protoPayload:
'@type': type.googleapis.com/google.cloud.audit.AuditLog
authenticationInfo:
principalEmail: overseer-kcc-tester@cnrm-barni-4.iam.gserviceaccount.com
authorizationInfo:
- granted: true
permission: serviceusage.services.enable
resource: projectnumbers/600845353393/services/mapmanagement.googleapis.com
methodName: google.api.serviceusage.v1.ServiceUsage.EnableService
request:
name: projects/cnrm-barni-4/services/mapmanagement.googleapis.com
resourceName: projects/cnrm-barni-4/services/mapmanagement.googleapis.com
response:
service:
config:
name: mapmanagement.googleapis.com
title: Map Management API
state: ENABLED
serviceName: serviceusage.googleapis.com |
|
[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 |
|
🤖 AI Factory started investigating CI check failures for this pull request. |
7ce7210 to
7689b8e
Compare
Investigating CI/CD build & test failuresRun: 31925207752 Investigating
|
|
🤖 AI Factory started reviewing this pull request in a sandbox. |
reviewbot-robot
left a comment
reviewbot-robot
left a comment
There was a problem hiding this comment.
KCC Auto-Review Results
- Trigger criteria matched: Yes
- Proto Diffs & Update Mask: Pass
- Structured Reporting: Pass
- KRM Status Updates: Fail (The controller skips KRM status updates when there are no-op updates)
- LRO Wait: Pass (Create and Update APIs are synchronous)
- Test Coverage: Pass (Both minimal and maximal E2E fixtures are present and verify all fields)
Detailed Findings / Actions Required:
- In
pkg/controller/direct/mapmanagement/mapmanagementmapconfig_controller.goat line 211, theUpdatemethod returnsnilon a no-op update without invokingupdateStatusto update KRM status. Please update this to callreturn a.updateStatus(ctx, updateOp, a.actual).
| return err | ||
| } | ||
|
|
||
| if !diffs.HasDiff() { |
There was a problem hiding this comment.
The controller MUST always update the KRM status at the end of Update operations, even if there are no fields that need updating in the GCP resource (no-op update).
Please replace return nil with return a.updateStatus(ctx, updateOp, a.actual).
|
🤖 AI Factory started addressing review feedback for this pull request. |
7689b8e to
19e270d
Compare
|
I have addressed the KCC Auto-Review feedback for the
The changes have been validated locally and force-pushed to the Pull Request branch. (This comment was generated by overseer) |
|
🤖 AI Factory started reviewing this pull request in a sandbox. |
reviewbot-robot
left a comment
reviewbot-robot
left a comment
There was a problem hiding this comment.
KCC Auto-Review Results
- Trigger criteria matched: Yes
- Proto Diffs & Update Mask: Pass (Calculates diffs and field masks cleanly using standard helpers
OnlySpecFieldsandDiffForTopLevelFields) - Structured Reporting: Pass (Logs spec differences before performing an update via
ReportDiff) - KRM Status Updates: Pass (The status is always updated at the end of both Create and Update operations, including no-op reconciliations)
- LRO Wait: Pass (Not applicable as the Google Maps Map Management API v2beta is synchronous and does not return LROs)
- Test Coverage: Pass (Minimal and maximal test cases are provided with recorded HTTP logs, covering all spec and status fields and updating the exceptions list successfully)
Detailed Findings / Actions Required:
- In
pkg/controller/direct/mapmanagement/mappers.go,poiBoostLevelis mapped usingdirect.LazyPtrin both directions. Since0is a valid, meaningful value for this field representing standard POI density, wrapping it inLazyPtrincorrectly treats0as an empty/nil pointer (due to it being the default value forint32), discarding the user's configuration. Please usedirect.PtrToinstead ofLazyPtrfor this field.
| out := &krm.MapFeatures{} | ||
| out.SimpleFeatures = direct.EnumSlice_FromProto(mapCtx, in.GetSimpleFeatures()) | ||
| if in.PoiBoostLevel != nil { | ||
| out.PoiBoostLevel = direct.LazyPtr(in.GetPoiBoostLevel()) |
There was a problem hiding this comment.
Using direct.LazyPtr here will cause poiBoostLevel to be dropped and treated as nil if the value is 0 (since 0 is the default/zero value of int32). However, 0 is a valid and meaningful value for this field (representing standard/no boosting of POIs). Use direct.PtrTo(in.GetPoiBoostLevel()) instead to correctly preserve 0.
| out := &pb.MapFeatures{} | ||
| out.SimpleFeatures = direct.EnumSlice_ToProto[pb.MapFeatures_SimpleFeature](mapCtx, in.SimpleFeatures) | ||
| if in.PoiBoostLevel != nil { | ||
| out.PoiBoostLevel = direct.LazyPtr(direct.ValueOf(in.PoiBoostLevel)) |
There was a problem hiding this comment.
Using direct.LazyPtr here will cause poiBoostLevel to be dropped and treated as nil if the value is 0 (since 0 is the default/zero value of int32). However, 0 is a valid and meaningful value for this field (representing standard/no boosting of POIs). Use direct.PtrTo(direct.ValueOf(in.PoiBoostLevel)) instead to correctly preserve 0.
|
🤖 AI Factory started addressing review feedback for this pull request. |
…ntMapConfig Fix: 11852
19e270d to
4ceac34
Compare
|
I have addressed the KCC Auto-Review feedback regarding the The field
This ensures that a value of (This comment was generated by overseer) |
|
🤖 AI Factory started reviewing this pull request in a sandbox. |
reviewbot-robot
left a comment
reviewbot-robot
left a comment
There was a problem hiding this comment.
KCC Auto-Review Results
- Trigger criteria matched: Yes
- Proto Diffs & Update Mask: Pass (Calculated correctly in
compare()usingcommon.DiffForTopLevelFieldsand passed as standard update payload). - Structured Reporting: Pass (Diffs are properly reported via
structuredreporting.ReportDiffbefore the update payload). - KRM Status Updates: Pass (Status updates are correctly performed at the end of both
CreateandUpdateoperations). - LRO Wait: Pass (The Google Maps MapManagement API's
CreateMapConfigandUpdateMapConfigendpoints are synchronous and return 200 OK with the resource directly; LRO Wait is not required). - Test Coverage: Pass (Minimal and maximal test cases are fully provided and verified; maximal test covers all KRM spec fields).
Detailed Findings / Actions Required:
No critical findings or actions required. The implementation is highly idiomatic, clean, and complies with all Config Connector engineering standards.
|
🤖 AI Factory started addressing review feedback for this pull request. |
|
I have reviewed the latest status of the The latest auto-review results indicate that all checks are fully passing:
All feedback (including status updates and The PR is fully mergeable and ready for final human owner review and approval. (This comment was generated by overseer) |
4 participants
Fixes #11852
This PR was generated by the overseer,overseer,greenfield,step/controller agent (powered by the gemini-3.5-flash model).
GCP Recording Details
The
./hack/record-gcptool was successfully run against real GCP using the projectcnrm-barni-4to generate the HTTP traffic logs (_http.log) and KRM object state golden files for both the minimal and maximal test fixtures.