flesh out READMEs and contributing docs #6
Conversation
k8s-ci-robot
requested review from
KnVerey,
jeremyrickard,
mengqiy and
yuwenma
k8s-ci-robot
added
cncf-cla: yes
| # TODO: Add CI to validate the metadata here. | ||
|
|
||
| apiVersion: config.kubernetes.io/v1alpha1 | ||
| kind: KRMFunctionDefinition |
There was a problem hiding this comment.
This is based on what the KrmFunctionDefinition spec will look like after kubernetes/enhancements#3220 goes in
CONTRIBUTING.md
Outdated
|
|
||
| Within the publisher's directory, there should be: | ||
| - An OWNERS file to approve KRM function metadata changes for the publisher. | ||
| - A directory for each published KRM function. This KRM function directory should contain a single file, |
There was a problem hiding this comment.
Why a directory with a single file instead of just the file directly under publishers?
There was a problem hiding this comment.
I like that a directory with a single file is very clear about what the file is, e.g. the file render-helm-chart/krm-function-metadata.yaml is very clearly the function metadata file for the render-helm-chart function. However I suppose this clarity can be equally accomplished by making it clear in the publishers README that this directory contains files of function metadata, where each file is the function name, e.g. render-helm-chart.yaml.
I have made the change to do the latter.
publishers/communities/sig-cli/render-helm-chart/krm-function-metadata.yaml
Outdated
Show resolved
Hide resolved
| image: us.gcr.io/k8s-artifacts-prod/krm-functions/render-helm-chart:unstable | ||
| requireNetwork: true | ||
| requireStorageMount: true | ||
| schema: |- |
There was a problem hiding this comment.
In the KEP we have openAPIV3Schema under this like CRD does, and then of course the schema is in OpenAPI V3 format. Is there a particular reason you're using Swagger 2.0 here?
There was a problem hiding this comment.
Ah I must have misread the KEP. I have updated it accordingly.
natasha41575
added
the
tide/merge-method-squash
k8s-ci-robot
added
size/L
| ### Repo layout | ||
|
|
||
| ``` | ||
| ├── publishers # Home for all functions metadata |
There was a problem hiding this comment.
I'd suggest the following layout to help publishing catalogs.
├── publishers # Home for all functions metadata
│ ├── kustomize
│ │ ├── functions
│ │ │ ├── fn-foo.yaml
│ │ │ ├── fn-bar.yaml
│ │ │ └── README.md
│ │ ├── catalogs
│ │ │ ├── v20220225.yaml
│ │ │ ├── v20220101.yaml
│ │ │ └── README.md
│ │ └── OWNERS # OWNERS of the publisher
...
@KnVerey WDYT?
There was a problem hiding this comment.
Hmm, yeah we didn't address where the static catalogs would go in the KEP, but it makes sense for them to be subjected to the same owners files as the functions themselves, so that layout looks good. I'd stick to a single README at the publishers/kustomize level though.
| └── OWNERS | ||
| ``` | ||
|
|
||
| ## Contributing in-tree KRM function source code |
There was a problem hiding this comment.
We should talk about the criteria to accept functions as in-tree functions. For example, we should not accept 3rd party functions as in-tree functions, since if they give up the maintenance, it will be a problem for us.
There was a problem hiding this comment.
What do we consider 3rd party functions? If some company wishes to donate a function, does it need to be under SIG-CLI? In that case, would there be any publishers in the krm-functions folder outside of krm-functions/sig-cli?
There was a problem hiding this comment.
I added a requirements section to krm-functions/README.md stating that we cannot accept 3rd party functions as in-tree functions
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengqiy, natasha41575 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 |
k8s-ci-robot
added
the
approved
* flesh out READMEs and contributing docs * suggested changes * suggested changes
4 participants
/cc @mengqiy
/cc @jeremyrickard
/cc @KnVerey
/cc @yuwenma