Skip to content

Conversation

@vincepri
Copy link
Member

@vincepri vincepri commented Feb 6, 2020

Signed-off-by: Vince Prignano [email protected]

/assign @gerred @DirectXMan12

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vincepri
To complete the pull request process, please assign mengqiy
You can assign the PR to them by writing /assign @mengqiy in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 6, 2020
@DirectXMan12
Copy link
Contributor

Context: we've resisted doing this so far, since it's something that's generally pretty easy to do with kustomize and friends, and it's not really generally an attribute inherent to the CRD (i.e. it's something you might want to change w/o changing your Go code).

What's the motivating usecase here?

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2020
@vincepri
Copy link
Member Author

Adding the marker allows for the CRDs in crd/bases to be populated with the label, rather than using kustomize to do add the label (which yeah it's pretty straightforward).

This is useful in envtest for example, we require a specific label to be present on the CRD when parsing object references.

This isn't a release blocker for us, we found another way (hardcode a test CRD).

@DirectXMan12
Copy link
Contributor

I think this generally encourages things that are bad patterns IMO, and the workarounds are pretty easy (kustomize, loading manually and setting the field, using a client to set the field, etc), so I'd prefer to not do this.

@vincepri
Copy link
Member Author

Sounds good to me, thanks for providing an explanation.

/close

@k8s-ci-robot
Copy link
Contributor

@vincepri: Closed this PR.

In response to this:

Sounds good to me, thanks for providing an explanation.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants