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

KEP-2906: Update Catalog schema for CRD compatibility #3220

Merged
merged 1 commit into from
Feb 23, 2022

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Feb 9, 2022

  • One-line PR description: Small update to the schema for function definitions to be compatible with CRD by ensuring that where they have the same fields, they are in the same places.

/cc @mengqiy @jeremyrickard

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 9, 2022
@jeremyrickard
Copy link
Contributor

I think this looks good. I approve, but I'll drop a hold because I think my approval will auto merge it based on my enhancements role. And other reviewer, if you approve feel free to drop the hold

/lgtm

/hold for others to review

@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 10, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2022
@@ -743,30 +788,32 @@ definitions:
type: array
items:
type: string
KRMFunction:
KRMFunctionDefinition:
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make the same changes to https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/2985-public-krm-functions-registry, right? It leverages the KRMFunction definition from here.

Copy link
Member

Choose a reason for hiding this comment

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

We will need to update it if we change the name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call--the names.kind change needs to be made there as well.

keps/sig-cli/2906-kustomize-function-catalog/README.md Outdated Show resolved Hide resolved
keps/sig-cli/2906-kustomize-function-catalog/README.md Outdated Show resolved Hide resolved
keps/sig-cli/2906-kustomize-function-catalog/README.md Outdated Show resolved Hide resolved
@@ -743,30 +788,32 @@ definitions:
type: array
items:
type: string
KRMFunction:
KRMFunctionDefinition:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to change the name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of strengthening the analogy to CRD, I though it would be useful to include "Definition" in the name of this type. In the same way that the CRD kind defines a CR rather than being one itself, this type defines a function. Does that make sense?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2022
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, mengqiy

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

@KnVerey
Copy link
Contributor Author

KnVerey commented Feb 23, 2022

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit f7583a7 into kubernetes:master Feb 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants