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

test: fix list fields detection in TestCRDFieldPresenceInUnstructured #3651

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

jingyih
Copy link
Collaborator

@jingyih jingyih commented Feb 11, 2025

Unblocks #3632

Fixed the incorrect detection of list fields in TestCRDFieldPresenceInUnstructured.

e.g.

=== FAIL: tests/apichecks TestCRDFieldPresenceInUnstructured (6.00s)
    utils.go:149: FAIL: unexpected diff in testdata/exceptions/missingfields.txt:   (
          	"""
          	... // 2370 identical lines
          	[missing_field] crd=logginglogsinks.logging.cnrm.cloud.google.com version=v1beta1: field ".spec.exclusions[].filter" is not set in unstructured objects
          	[missing_field] crd=logginglogsinks.logging.cnrm.cloud.google.com version=v1beta1: field ".spec.exclusions[].name" is not set in unstructured objects
        + 	[missing_field] crd=managedkafkaclusters.managedkafka.cnrm.cloud.google.com version=v1beta1: field ".spec.gcpConfig.accessConfig.networkConfigs[].subnetworkRef" is not set; neither 'external' nor 'name' are set
          	[missing_field] crd=memcacheinstances.memcache.cnrm.cloud.google.com version=v1beta1: field ".spec.maintenancePolicy.createTime" is not set in unstructured objects
exit status 1
          	[missing_field] crd=memcacheinstances.memcache.cnrm.cloud.google.com version=v1beta1: field ".spec.maintenancePolicy.description" is not set in unstructured objects
          	... // 1517 identical lines
          	"""
          )

@jingyih
Copy link
Collaborator Author

jingyih commented Feb 11, 2025

/assign @acpana

@jingyih
Copy link
Collaborator Author

jingyih commented Feb 11, 2025

Fixes #3571

@jingyih
Copy link
Collaborator Author

jingyih commented Feb 11, 2025

/cc @jasonvigil

@google-oss-prow google-oss-prow bot requested a review from jasonvigil February 11, 2025 06:35
Copy link
Collaborator

@acpana acpana left a comment

Choose a reason for hiding this comment

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

thank you loads for taking this on 💯

I spotted checked some of the newly fixed exceptions and can confirm that all fixes in #3636 are present here too!

good job! 🥇

Comment on lines +539 to +542
if itemMap, ok := item.(map[string]interface{}); ok {
if hasField(itemMap, remainingPath) {
return true // found the field in one of the items, we can stop searching
}
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 may be "double checking" field paths here in the case where a remaining path is not actually present anywhere else. We check it here in the small loop and then again in the outer loop.

But if that's the case we can live with that for now 💯 .

@google-oss-prow google-oss-prow bot added the lgtm label Feb 11, 2025
@jasonvigil
Copy link
Collaborator

/lgtm
thank you @jingyih

@jasonvigil
Copy link
Collaborator

/assign yuwenma

@yuwenma
Copy link
Collaborator

yuwenma commented Feb 11, 2025

/lgtm

@jasonvigil
Copy link
Collaborator

This PR isn't merging for some reason. I am guessing perhaps because @acpana used the github method of approving the PR, rather than the oss-prow commands. I will try to approve also, to see if that un-sticks it.

@jasonvigil
Copy link
Collaborator

Hmm, doesn't seem to have worked. Looks like it was fine before, we just need approval from @yuwenma

@cheftako
Copy link
Collaborator

/approve

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acpana, cheftako, jasonvigil

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 5af3b35 into GoogleCloudPlatform:master Feb 11, 2025
17 of 18 checks passed
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.

5 participants