Skip to content

Add support for Shared Reservations in ComputeReservation#6829

Open
codebot-robot wants to merge 5 commits intoGoogleCloudPlatform:masterfrom
codebot-robot:support-shared-reservations
Open

Add support for Shared Reservations in ComputeReservation#6829
codebot-robot wants to merge 5 commits intoGoogleCloudPlatform:masterfrom
codebot-robot:support-shared-reservations

Conversation

@codebot-robot
Copy link
Collaborator

This PR enables support for shared reservations in 'ComputeReservation' by enabling the 'share_settings' field.

Key changes:

  • Removed 'share_settings' from 'ignoredFields' for 'ComputeReservation' in 'config/servicemappings/compute.yaml'.
  • Updated the 'ComputeReservation' CRD to include the 'shareSettings' field.
  • Added a new sample for shared reservations in 'config/samples/resources/computereservation/shared-compute-reservation/'.
  • Added a basic mock implementation for the Reservations service in 'mockgcp' to support future testing of this feature.

This PR was generated by the gemini-3-flash-preview model.

Fixes #6462

This change enables 'share_settings' for 'ComputeReservation' by removing it from 'ignoredFields' in the service mapping and updating the CRD.
A new sample for shared reservations is also added.
A basic mock implementation for Reservations is added to mockgcp to support testing.

Fixes GoogleCloudPlatform#6462
@google-oss-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maqiuyujoyce for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Collaborator Author

@codebot-robot codebot-robot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a nil check to the mock implementation and included a new test fixture for shared reservations to ensure better coverage and reliability. I also verified the CRD structure against the latest service mapping.

Copy link
Collaborator Author

@codebot-robot codebot-robot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR to address the feedback:

  1. Added resource references (idRef and projectIdRef) to 'ComputeReservation' in the service mapping and updated the CRD accordingly.
  2. Updated the sample and test fixture to use these new reference fields.
  3. Enhanced the mock implementation with better validation and nil checks for 'ShareSettings'.
  4. Reorganized test fixtures to follow standard conventions.

- Regenerate manifests and Go types.
- Fix test case names in resourceContextMap to match testdata leaf directories.
- Fix sharedreservation test data to use 'idRef' and 'projectIdRef'.
- Add acronym and missing field exceptions for 'projectIdRef' in ComputeReservation.
@codebot-robot
Copy link
Collaborator Author

--- INVESTIGATION REPORT ---
Run ID: 22466128315
Name: Presubmit
Cause: Code Error
Details: Manifests were not regenerated.
Action Taken: Regenerated manifests and Go types.

Run ID: 22466128327
Name: ci-presubmit
Cause: Code Error / Test Failure
Details:

  1. TestValidTestName failed because the test case name 'computereservation' did not match the leaf directory names 'basicreservation' and 'sharedreservation'.
  2. TestCRDsAcronyms failed because 'projectIdRef' should be 'projectIDRef' (renamed back to projectIdRef to match ComputeNodeGroup and added exceptions).
  3. TestCRDFieldPresenceInTests failed because 'idRef' and 'projectIdRef' were not correctly set in the sharedreservation test case (used incorrect names 'id' and 'projectId').
    Action Taken:
  4. Updated pkg/test/resourcefixture/contexts/compute_context.go with correct test case names.
  5. Added acronym exceptions for 'projectIdRef' in ComputeReservation.
  6. Fixed sharedreservation test data to use correct field names.
  7. Added missing field exceptions for ComputeReservation.

Run ID: 22460289713
Name: ci-presubmit
Cause: Code Error / Test Failure
Details: Similar to Run ID 22466128327.
Action Taken: Fixed as part of the overall changes.

Run ID: 22458657112
Name: ci-presubmit
Cause: Code Error / Test Failure
Details: Similar to Run ID 22466128327.
Action Taken: Fixed as part of the overall changes.

@codebot-robot
Copy link
Collaborator Author

--- INVESTIGATION REPORT ---
Run ID: 22498216249
Name: ci-presubmit
Cause: Code Error
Details: TestCRDsAcronyms and TestCRDFieldPresenceInTests failed due to incorrect sorting in acronyms.txt and stale entries in missingfields.txt (fields are now present in tests).
Action Taken: Updated exception files using WRITE_GOLDEN_OUTPUT=1 go test ./tests/apichecks.

Run ID: 22498216197
Name: Presubmit
Cause: Code Error
Details: Manifests were not matching the expected output (specifically projectIdRef was being removed from required fields).
Action Taken: Regenerated manifests using make manifests.

Run ID: 22466128315
Name: Presubmit
Cause: Code Error
Details: Manifests were not regenerated.
Action Taken: Fixed in subsequent commits.

Run ID: 22466128327
Name: ci-presubmit
Cause: Code Error / Test Failure
Details:

  1. TestValidTestName failed because the test case name 'computereservation' did not match the leaf directory names 'basicreservation' and 'sharedreservation'.
  2. TestCRDsAcronyms failed because projectIdRef should be projectIDRef (renamed back to projectIdRef to match ComputeNodeGroup and added exceptions).
  3. TestCRDFieldPresenceInTests failed because idRef and projectIdRef were not correctly set in the sharedreservation test case.
    Action Taken: Updated compute_context.go with correct names, added acronym exceptions, and fixed test data.

Run ID: 22460289713
Name: ci-presubmit
Cause: Code Error / Test Failure
Details: Similar to Run ID 22466128327.
Action Taken: Fixed as part of the overall changes.

Run ID: 22458657112
Name: ci-presubmit
Cause: Code Error / Test Failure
Details: Similar to Run ID 22466128327.
Action Taken: Fixed as part of the overall changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Shared Reservations

1 participant