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

feat: Turn on stricter golden log check in e2e fixture (part 1) #1934

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

yuwenma
Copy link
Collaborator

@yuwenma yuwenma commented Jun 4, 2024

Description

This PR adds the golden object log and mock http log to all existing mocked GCP resources. Once it goes in, we can turn on the presubmit check in #2061

Known Caveats

We have not tuned all the existing Mock GCP to make its log output the same as the real GCP log yet, which means these mock gcp server may need improvements during development, and the golden log needs to be re-generated if necessary.

Requirements for ConfigConnector Contributors

We want to turn on this golden log check now and expect it to be used (and maintained) in the following cases:

  1. New PRs should give stable golden log output before they can merge in. (Reasonable golden log change is expected, and golden log should be re-generated then)
  2. MockGCP coverage is required for adding a new field or a new resource (i.e. add/modify a field, add a resource). When adding a single field, if the MockGCP does not exist yet, developers should add the corresponding MockGCP coverage for the entire resource.
  3. MockGCP should be continuously maintained at the same time as adding new resource, moving resource from TF/DCL based controller to the Direct Controller

@justinsb
Copy link
Collaborator

justinsb commented Jun 4, 2024

So I don't know if all our tests are consistent with record-gcp yet.

But ... I like this idea ... we can at least require that compare-mock produces stable output, and verify it!

@yuwenma
Copy link
Collaborator Author

yuwenma commented Jun 4, 2024

So I don't know if all our tests are consistent with record-gcp yet.

But ... I like this idea ... we can at least require that compare-mock produces stable output, and verify it!

In this PR, I am trying to see how big the difference is and how much work need to be done to turn on the strict http log check.

@justinsb
Copy link
Collaborator

justinsb commented Jun 7, 2024

I've been thinking about this ... you're right, we should have done this from the start. It might be that our mockgcp results aren't 100% equal to GCP in all cases. But we still get a lot more signal by checking these values against mockgcp than by not checking them!

@yuwenma yuwenma force-pushed the enable-strict-e2e branch from 2d99f8f to 53be30c Compare June 19, 2024 23:09
@yuwenma yuwenma changed the title [WIP] Turn on stricter golden log check in e2e fixture and samples. Turn on stricter golden log check in e2e fixture and samples. Jun 20, 2024
@yuwenma yuwenma changed the title Turn on stricter golden log check in e2e fixture and samples. feat: Turn on stricter golden log check in e2e fixture and samples (part 1) Jun 20, 2024
@yuwenma yuwenma changed the title feat: Turn on stricter golden log check in e2e fixture and samples (part 1) feat: Turn on stricter golden log check in e2e fixture (part 1) Jun 20, 2024
@@ -24,7 +24,7 @@ cd ${REPO_ROOT}/
echo "Downloading envtest assets..."
export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path)

export KCC_USE_DIRECT_RECONCILERS=MonitoringDashboard
KCC_USE_DIRECT_RECONCILERS=MonitoringDashboard
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need export, otherwise it doesn't apply to subsequent requests, but as this is passing tests let's go with it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fix this in Part 2. Want to use this as an example to highlight some changes.

@google-oss-prow google-oss-prow bot added the lgtm label Jun 20, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

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