KEP-32: Community Repository Management #1574
Conversation
Detail a workflow for upstream operator developers to add or update operator packages in the community repository by referencing their upstream package. Signed-off-by: Jan Schlicht <jan@d2iq.com>
| tag: "1.0.0" | ||
| dir: operator | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Should we be responsible for compiling the operator as part of the KUDO community repo, or should we be referencing releases that are built and hosted on GitHub (e.g. like Krew does https://github.com/kubernetes-sigs/krew-index/blob/master/plugins/kudo.yaml)?
By referencing a release, we could eventually support various release objects (e.g. helm charts/Docker Images/OCI Artifact Specs/etc) rather than requiring the internal objects to build those objects.
There was a problem hiding this comment.
It depends on what "compiling" should cover. We should be responsible for creating tarballs from operator package folders because these tarballs are specific to repositories and operator developers shouldn't keep them as part of their Git repositories. Though we could also support these tarballs directly if developers want to do that.
There was a problem hiding this comment.
Agreed. Creating the tarballs ourself also makes sure that they're not changed afterwards
There was a problem hiding this comment.
I'm ok with having a model that creates the tarballs... but I also want to support referencing prebuilt tars. Requiring us to build them assumes that the build process is exactly how we define it... and that the git repo has an instance of this... I could imagine additional templates or build techniques for which the files to create the operator are an intermediary step (which an org may not want to check-in, outside of our requirement on them). Supporting prebuilts is necessary IMO. We should: 1) either make this a current non-goal (for future definition) or 2) define it.
There was a problem hiding this comment.
The use of "version" concerns me a bit...
context: The version of an operator is defined in the operator.yaml and is part of the tarball name. The tarball version also defines this as app_version and op_version. so...
- Is the "version" used here... app, or op?
- Is this expected to be the app_V-op_v?
- Is it checked / verified against the artifact? does the PR fail if it doesn't match?
Additionally... can we make it easy for people... can we assume that the "tag" is the "version" if not specified? It seems like we could assume that the version is the tag but allow tag to override that assumption.
There was a problem hiding this comment.
Each package that is to be added to the community repo needs to be checked for validity by the CI. This would at least cover that it is recognized by KUDO as an operator package. More checks for conformance are of course possible as well. All of these checks will work with directories as well as tarballs. When referencing tarballs, these will be copied into the community repository. I'll clarify this in the KEP.
@kensipe Good point regarding the version. This is again a question on which metadata should in these reference files versus what metadata is provided by the actual operator package. A reference file could be as simple as
versions:
- git:
repository: github.com/example/example-operator.git
tag: "1.0.0"
dir: operator
because the other metadata (name as well as version) is redundant. But IMO there's value to have them duplicated here, as it helps developers to understand what is being referenced. These values will show up in CI logs and could help debug CI failures, e.g. when using a wrong Git tag.
| dir: operator | ||
| ``` | ||
|
|
||
| Once this PR is merged, CI tooling detects the new YAML file, clones the referenced upstream Git repository, checks out the tag, and adds the operator package in the specified folder to the existing index. Of course, CI tests that don't update the community repository can run before the PR is merged. This workflow is similar to [krew-index](https://github.com/kubernetes-sigs/krew-index) |
There was a problem hiding this comment.
Should we define the testing model? E.g. do we run tests if we find a kuttl-test.yaml in the upstream repo?
There was a problem hiding this comment.
I left testing out, this needs to be discussed. IMO, upstream operators should be responsible for testing. The community repo tooling should only be concerned with testing for conformance which should run tests that work for every operator.
There was a problem hiding this comment.
Agree that the larger conversation around testing should be discussed and implemented... and that we don't need to include it here (for now). But shouldn't we cover:
- failures in access to bucket (network or auth)
- does the PR merge if the failure occurred? IS there a notification if this failure occurs?
- Is there a check on version? is that possible to fail? and what happens? / how is it resolved?
- What is github is down? where the git clone fails? or the repository is deleted?
There was a problem hiding this comment.
Aren't these points implementation details? Not sure if they should be covered here. IMO, we need to distinguish between conformance tests that are independent of an operator -- e.g., that the referenced Git repo can be cloned, the referenced operator is valid, ... -- and tests specific to an operator -- e.g. a kuttl-test.yaml specified by metadata.
There was a problem hiding this comment.
I would really like to see tests here.
We should ask for minimal tests with a timeout of 5 min and we could use them to perform sanity before merging PRs
For example, a default version of an operator might fail due to prometheus operator requirement, so doing simple installation might not be an answer here. But we shouldn't explore and discover how to do a minimal installation that works, that should be provided by operator developer. And tests that consume a limited amount of resources and with a reasonable timeout would help us improve a bit the quality here.
There was a problem hiding this comment.
I'm a fan of moving forward. there is need for more clarification as noted... but It seems good enough to push forward.
In addition to other comments:
- what is the mechanism for removing an operator? either a mistaken version or a full suite of versions of an operator? If we are not going to define that we should have it as a non-goal.
| tag: "1.0.0" | ||
| dir: operator | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I'm ok with having a model that creates the tarballs... but I also want to support referencing prebuilt tars. Requiring us to build them assumes that the build process is exactly how we define it... and that the git repo has an instance of this... I could imagine additional templates or build techniques for which the files to create the operator are an intermediary step (which an org may not want to check-in, outside of our requirement on them). Supporting prebuilts is necessary IMO. We should: 1) either make this a current non-goal (for future definition) or 2) define it.
| tag: "1.0.0" | ||
| dir: operator | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The use of "version" concerns me a bit...
context: The version of an operator is defined in the operator.yaml and is part of the tarball name. The tarball version also defines this as app_version and op_version. so...
- Is the "version" used here... app, or op?
- Is this expected to be the app_V-op_v?
- Is it checked / verified against the artifact? does the PR fail if it doesn't match?
Additionally... can we make it easy for people... can we assume that the "tag" is the "version" if not specified? It seems like we could assume that the version is the tag but allow tag to override that assumption.
| dir: operator | ||
| ``` | ||
|
|
||
| Once this PR is merged, CI tooling detects the new YAML file, clones the referenced upstream Git repository, checks out the tag, and adds the operator package in the specified folder to the existing index. Of course, CI tests that don't update the community repository can run before the PR is merged. This workflow is similar to [krew-index](https://github.com/kubernetes-sigs/krew-index) |
There was a problem hiding this comment.
Agree that the larger conversation around testing should be discussed and implemented... and that we don't need to include it here (for now). But shouldn't we cover:
- failures in access to bucket (network or auth)
- does the PR merge if the failure occurred? IS there a notification if this failure occurs?
- Is there a check on version? is that possible to fail? and what happens? / how is it resolved?
- What is github is down? where the git clone fails? or the repository is deleted?
Signed-off-by: Jan Schlicht <jan@d2iq.com> Co-authored-by: Ken Sipe <kensipe@gmail.com> Co-authored-by: Thomas Runyon <runyontr@gmail.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
There was a problem hiding this comment.
this is looking great @nfnt !
I have few comments but can be part of another iteration of the KEP as for today.
- I would like to make mandatory the minimal tests
- And maybe we should leave the goal
Provide a mechanism to define upstream operator package maturityout of this KEP? As the scope of that can be good enough to make its own KEP.
I don't think the maturity level should be some open field as metadata. And each contributor to the community repository would know the criteria which would make its operator jump to the next maturity level.
Signed-off-by: Jan Schlicht <jan@d2iq.com> Co-authored-by: Zain Malik <zmalikshxil@gmail.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
There was a problem hiding this comment.
I am still very concerned with version. @nfnt commented the value in metadata... I agree with surfacing metadata... I think it is very confusing and will be an issue.
This metadata isn't generated... it is created by the human that wants to add an operator... it is very unclear what "version" to put here. This is consistent with our teams heavy discussion around versions in operators months ago. Is it the operator version... or underlying tech version... or the combo of those? and do we expect humans to remember the order of opv-techv or techv-opv? If I'm not clear on what this version is... how can we expect newcomers?
The only left to do is look at all the other examples and try to mimic that...
It feels like clarification of version... what it is and how it is to be used is needed
|
Good points @kensipe! Let's follow KEP-19 here and use a (required) |
Version metadata is provided as described in KEP-19. Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
What this PR does / why we need it:
Detail a workflow for upstream operator developers to add or update operator packages in the community repository by referencing their upstream package.