Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update compute mocks #2390

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

gemmahou
Copy link
Collaborator

Change description

TF uses compute/beta/ endpoint, to avoid log diffs after migrating to Scifi, I made below changes:

The urls should be v1 https://compute.googleapis.com/compute/v1/
The self links should have format https://www.googleapis.com/compute/v1/...

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@justinsb
Copy link
Collaborator

Could you start the direct controller using the same API version as the existing controllers?

@gemmahou
Copy link
Collaborator Author

Could you start the direct controller using the same API version as the existing controllers?

Per this comment I guess we want to switch to v1 eventually?
Are you suggesting using beta version as the existing controller for easier testing and ensure backward compatibility?

Other than the API version, the selflink format in compute mocks are inconsistent(should be https://www.googleapis.com/compute/v1, b/131629538), I can address that in this PR.

@justinsb
Copy link
Collaborator

justinsb commented Aug 1, 2024

Are you suggesting using beta version as the existing controller for easier testing and ensure backward compatibility?

Right - I think that should be easier. Otherwise I think all our tests will start failing with the TF-based controllers?

// Terraform uses the /beta/ endpoints, but mocks and direct controller should use /v1/
// This special handling to avoid diffs in http logs.
// This can be removed once all Compute resources are migrated to direct controller.
basePath := "https://compute.googleapis.com/compute"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see! I guess this is a nice trick also. @yuwenma what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

works for me.

obj.CreationTimestamp = PtrTo(s.nowString())
obj.Id = &id
obj.Kind = PtrTo("compute#forwardingRule")
obj.LabelFingerprint = PtrTo("abcdef0123A=")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'd like to add the real logic on label fingerprint in the mockGCp and replace the value in normalize.go

// Terraform uses the /beta/ endpoints, but mocks and direct controller should use /v1/
// This special handling to avoid diffs in http logs.
// This can be removed once all Compute resources are migrated to direct controller.
basePath := "https://compute.googleapis.com/compute"
Copy link
Collaborator

Choose a reason for hiding this comment

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

works for me.

@yuwenma
Copy link
Collaborator

yuwenma commented Aug 1, 2024

/lgtm
/approve

Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit fd40b47 into GoogleCloudPlatform:master Aug 1, 2024
13 checks passed
@gemmahou gemmahou deleted the compute-mock branch August 1, 2024 21:38
@yuwenma yuwenma added this to the 1.121 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants