Skip to content

linters: add tests for labels#4884

Open
justinsb wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
justinsb:crd_lint_must_have_annotation
Open

linters: add tests for labels#4884
justinsb wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
justinsb:crd_lint_must_have_annotation

Conversation

@justinsb
Copy link
Collaborator

No description provided.

@justinsb
Copy link
Collaborator Author

/hold

We need to fix the lack of test coverage before we merge this one

Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

/approve

Thanks! For cnrm.cloud.google.com/system"="true", another option is to add it to the kustomization which applies to the operator CRDs, so that we can guarantee the annotation without coding (and test) effort. https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/config/crds/kustomization.yaml

@google-oss-prow
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

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

@yuwenma
Copy link
Collaborator

yuwenma commented Jul 30, 2025

We need to fix the lack of test coverage before we merge this one

I'm okay to add the label coverage in a follow-up PR, and the exception should be removed.

I like the controller-type exception list! But we have no quick fixes right now:

  • the "is not direct but has no TF or DCL labels" is due to missing direct controllers. The generate-types works well, but the direct controller has real issues, so it's not implemented yet
  • the "is direct but has TF label" is due to beta TF/DCL resource has the direct as opt-in.

@justinsb
Copy link
Collaborator Author

But we have no quick fixes right now:

Right, I think it's OK to have exceptions here. I'll add some text to make it look less scary.

@justinsb justinsb force-pushed the crd_lint_must_have_annotation branch from e6cf761 to 3948afc Compare July 31, 2025 23:35
@justinsb
Copy link
Collaborator Author

justinsb commented Jul 31, 2025

another option is to add it to the kustomization

We should be committing correct CRDs in the repo (and not committing intermediates): for tests, for customers, for AI

@justinsb
Copy link
Collaborator Author

/assign @yuwenma

@justinsb justinsb force-pushed the crd_lint_must_have_annotation branch from 3948afc to 2a17a88 Compare August 1, 2025 12:50
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.

2 participants