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

TEP for Catalog Test Requirements and Infra for Verified+ #170

Closed

Conversation

bobcatfish
Copy link
Contributor

@bobcatfish bobcatfish commented Aug 10, 2020

This is a WIP but I want to open it so folks can comment and I don't block anyone.

I've been porting this from the original test infra design, I'm currently in the section on Pipeline design, trying to be more specific about how exactly the Pipeline(s) would work and what Tasks are required.

TODO:

  • Add detailed list of Tasks required
  • Finish porting content of original design doc
  • Fill in user stories
  • Fill in risks + mitigations
  • Fill in performance
  • Fill in test plan
  • Fill in drawbacks
  • Fill in alternatives
  • Update TOC

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2020
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign bobcatfish
You can assign the PR to them by writing /assign @bobcatfish 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

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 10, 2020
@bobcatfish bobcatfish changed the title TEP for Catalog Verified + Official test requirements + infra TEP for Catalog Verified + Official test requirements and infra Aug 10, 2020
bobcatfish added a commit to bobcatfish/community that referenced this pull request Aug 12, 2020
We've already started work on this in the catalog so we could even go
straight to "implemented" if we wanted to, but since we haven't
implemented all of it (e.g. testing for verifiable and official tiers
via tektoncd#170) maybe "implementable" makes the most sense for now?
tekton-robot pushed a commit that referenced this pull request Aug 14, 2020
We've already started work on this in the catalog so we could even go
straight to "implemented" if we wanted to, but since we haven't
implemented all of it (e.g. testing for verifiable and official tiers
via #170) maybe "implementable" makes the most sense for now?
a. Linting and configuration tests should be applied to all resources
b. Testing should be applied regularly to all resources in the catalog
c. Folks developing resources should be able to easily run these tests themselves
d. It must be possible to apply this testing to resources that rely on external services (e.g. S3, slack)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

also want to make it possible to for anyone to submit resources to the catalog
with very little barrier in order to encourage contributions
1. They may initially be true of the catalog and later be true of the hub and not the catalog
2. Users should be able to define their own catalogs as well, and use resources from their own
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this one - catalog is just a git repo today right? If we're going to make it more "special" and integrated, that would probably be it's own TEP?

Copy link
Member

Choose a reason for hiding this comment

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

catalog(s) are just git repositories yes. I am also not sure about that item, as I feel this TEP is only about the tektoncd/catalog, not any other catalog. I think we should focus on tektoncd/catalog and only on it for now — once it works well, we can think bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good point - i think this is a requirement for the catalog + hub projects as a whole, maybe for the hub in particular.

im happy to remove it from here - the one thing that comes to mind is if this is laying out testing requirements for official + verified, how do you create verified tasks that exist in another catalog? but like @dlorenc said that's probably the material of a separate TEP

2. The most recent release
3. The nightly release
3. Verified Tasks have at least one test that can be executed to verify them
4. Tests are executed against PRs and periodically every evening
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just say daily? evening is timezone subjective :)

bogged down.
-->

#### Story 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you had two good ones in the intro - users should be able to rely on stuff working, and contributors should be able to add things easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, thanks! :D


For this to work well we'll need to propose and implement some features:

- [ ] Allow extra workspaces to be provided to a PipelineRun (this way we can provide
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean just extras that get ignored at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i think so 🤔

I was going to type an example here but i think it might be more useful in the TEP itself so I'll try to add it to the markdown file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would probably need its own TEP X'D


### Quality

1. If a resource is in the catalog, a user should feel confident that it will work as advertised
Copy link
Member

Choose a reason for hiding this comment

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

should we say "in the official catalog", and defining what is the official catalog ? (tektoncd/catalog would be the official, any other repository would not be considered as the "official" one)

also want to make it possible to for anyone to submit resources to the catalog
with very little barrier in order to encourage contributions
1. They may initially be true of the catalog and later be true of the hub and not the catalog
2. Users should be able to define their own catalogs as well, and use resources from their own
Copy link
Member

Choose a reason for hiding this comment

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

catalog(s) are just git repositories yes. I am also not sure about that item, as I feel this TEP is only about the tektoncd/catalog, not any other catalog. I think we should focus on tektoncd/catalog and only on it for now — once it works well, we can think bigger.

3. The nightly release
3. Verified Tasks have at least one test that can be executed to verify them
4. Tests are executed against PRs and periodically every evening
1. The nightly release test is probably only periodic
Copy link
Member

Choose a reason for hiding this comment

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

daily or "every evening" is also periodic 😝 I guess you mean a higher period, like weekly maybe ?

performance requirements.
-->

- number of clusters required; using kind instead, using sidecar registry, only testing what has changed
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to define two things:

  • what runs on PRs
  • what runs periodically

On PRs, we don't need to run all the tests, only the one affected by the changes — this speed things up. On periodics, we should run all.

* For example if we release `v1beta2` in `v0.17.0`, we would still support
`v1beta1` for [at least 9 months worth of releases](https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md#alpha-beta-and-ga)
and we could continue to test `v1beta1` Tasks against all releases in that time.
* Once we stop supporting an older API, we will have to decide if we want to keep
Copy link
Member

Choose a reason for hiding this comment

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

We need to define "we", but if tektoncd/pipeline stop supporting v1alpha1, catalog should stop running test against any v1alpha1 resources, and stop supporting those.

- name: base-version
value: "v3.2.1"

# This inline Task checks the results and side effects
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we need to add something about using results in the catalog recommandation 👼

Comment on lines +310 to +358
- name: setup-sample
runAfter: [clone-repo]
workspaces:
- name: kubeconfig
workspace: modifiable-cluster-kubeconfig
taskSpec:
workspaces:
- name: kubeconfig
steps:
- name: install-tekton
image: lachlanevenson/k8s-kubectl
script: |
# The Task will be interacting with Tekton, we need to install it
kubectl --kubeconfig $(workspaces.kubeconfig.path)/kubeconfig \
apply --filename https://storage.googleapis.com/tekton-releases/pipeline/previous/v0.10.0/release.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I think this shouldn't be part of the pipeline the user has to write, otherwise how do we support running against multiple version of pipeline ? I think we should provide a task that can be used to provision a cluster if needed (aka something smart that detect a tiny bit where it's running, and act based on it).

Copy link
Member

Choose a reason for hiding this comment

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

That task would know which pipeline version it targets through parameters. Which means we would need to define some "standard" parameters for the test pipeline (aka, we need to provide a test pipeline skeleton 🙃 )

Comment on lines +451 to +487
1. A Pipeline that determines which Tasks need to be tested and runs the second Pipeline for each.
a. For pull requests, this will be only the Tasks that have changed
b. For nightly runs, this will be all Tasks
Copy link
Member

Choose a reason for hiding this comment

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

This can be 2 independent pipeline, with common tasks 🙃

2. If this is because the Task was relying on functionality that was not actually part
of the official API, Tekton maintainers will have to fix this or consider marking the
Task as deprecated (maybe moving it to a deprecated folder?) and no longer supporting it.

Copy link
Member

Choose a reason for hiding this comment

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

There is a 3rd reason that a test would fail that was recently discovered. tektoncd/catalog#481 The catalog tests injects and mutates the yaml before applying it. Tests have been failing cause of some sidecar shenanigans that have been injected into the yaml. This probably will be flushed out in the Test Plan. Im not quite sure how this should be done.

We will iteratively provide more and more infrastructure for Tekton Tasks
to test against. At a minimum we will provide:

* An image registry (an instance of docker-registry running in the same namespace) that can be pushed to
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool to have a single docker registry in its own namespace that can be referenced by svc. registry.registry.svc.cluster.local. The only issue would be conflicting tags.

b. Verify requirements, including specifying the min version of Tekton Pipelines
2. Determine the the versions of Tekton Pipelines to test against
3. For each version to test this Task against, start the third Pipeline
3. A Pipeline which tests an individual Task against a version of Tekton Pipelines
Copy link
Member

Choose a reason for hiding this comment

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

I think requiring our pipelines to comprise of tasks that are already in the catalog would be less confusing for beginners. Also, it would reduce redundancy and improve the quality of our pipelines. It promotes smaller prs and tests will give an accurate indication on what is actually failing. I.E If the pipeline is assembled incorrectly, is the task failing because is there an edge case we did not account for. I feel like having a smorgasbord of tekton resources could lead to catastrophic failures.

@bobcatfish bobcatfish changed the title TEP for Catalog Verified + Official test requirements and infra TEP for Catalog Test Requirements and Infra for Verified+ Aug 18, 2020
@pritidesai
Copy link
Member

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Aug 20, 2020
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2020
@bobcatfish
Copy link
Contributor Author

Pushed an update and addressed some of @dlorenc 's feedback but didn't get to @vdemeester or @popcor255 yet 🙏

@bobcatfish
Copy link
Contributor Author

I have made so little progress on this that I'm going to close it so it's not always coming up in TEP reviews, etc. 😬

I still hope to someday get back to it but if anyone else wants to pick up the reins from here that would be very exciting!

My main hope is that we will eventually have a way to have "verified" items in the catalog such that:

  1. Folks submitting to the catalog can provide tests alongside their submissions (maybe in the meantime we consider what we currently have for tests as satisfying this?)
  2. These tests can be defined in Tekton Pipelines/Tasks themselves (this is the main missing piece)

@chmouel might be relevant to your interests ^^

@bobcatfish bobcatfish closed this Oct 26, 2020
@chmouel
Copy link
Member

chmouel commented Oct 26, 2020

@bobcatfish I think even it's not very pretty we are mostly there on the current test runner in catalog.

The main difference I see between this TEP and what we have, is that the current testrunner doesn't assume we have an external infrastructure setup it would try to embed the registries or other infra piece inside the task so that the Run can point to it.

I have documented here the different ways on how to do functional testing inside the catalog :

https://github.com/tektoncd/catalog/blob/master/CONTRIBUTING.md#end-to-end-testing

You can have spin up a Jenkins repository and have the Run pointing to it :

https://github.com/tektoncd/catalog/blob/master/task/jenkins/0.1/tests/pre-apply-task-hook.sh

Or you could 'mock' an API service with rules :

https://github.com/tektoncd/catalog/blob/master/task/github-add-comment/0.1/tests/fixtures/github-post-comment.yaml

and have the Run pointing to it :

https://github.com/tektoncd/catalog/blob/master/task/github-add-comment/0.1/tests/run.yaml

The part that is really not that pretty is how to add sidecars to a task dynamically from a Run without modifying the Task itself we can't do that with podTemplates or other methods so we have to manipulate the task itself with shell script before applying it. Maybe that could be a feature to add?

It's on my todo list to modify the testrunner to be able to test on different pipeline versions, a lot of it is actually going to be more about adding new jobs to plumbing. cc @vdemeester

@bobcatfish
Copy link
Contributor Author

@chmouel the other big difference from my perspective is that we are currently asking users to write scripts (pre-apply-task-hook.sh, pre-apply-taskrun-hook.sh) - I'd really like to get to a world where the tests and setup are defined in Tasks and Pipelines (even if those are wrappers around scripts).

The part that is really not that pretty is how to add sidecars to a task dynamically from a Run without modifying the Task itself we can't do that with podTemplates or other methods so we have to manipulate the task itself with shell script before applying it. Maybe that could be a feature to add?

The other way we could approach this is instead of having Task authors define runs, they could define Pipelines with a particular signature. e.g. in the case of https://github.com/tektoncd/catalog/blob/master/task/github-add-comment/0.1/tests/run.yaml instead of specifying the PipelineRun as well, we could generate that when we run the tests. We'd still need a way to inject the sidecar - I think tektoncd/pipeline#1838 (comment) will help with that.

jerop added a commit to jerop/community that referenced this pull request Jan 6, 2022
Catalog - we will explore the proposal in further changes.

The aim of this TEP is to establish support tiers for resources in the
Tekton Catalog to ensure users get high quality resources that they can
rely on while making it easy for contributors to submit resources to the
Tekton Catalog.

The goals of the TEP are to address the following aspects of the Tekton
Catalog:
- Ownership and Maintenance
- Automated Testing and Dogfooding
- Image Scanning for Common Vulnerabilities and Exposures (CVEs)
- Verified Remote Resources

Related PRs:
- tektoncd#494
- tektoncd#170
- tektoncd#495
jerop added a commit to jerop/community that referenced this pull request Jan 6, 2022
This change adds the problem statement for Support Tiers in the Tekton
Catalog - we will explore the proposal in further changes.

The aim of this TEP is to establish support tiers for resources in the
Tekton Catalog to ensure users get high quality resources that they can
rely on while making it easy for contributors to submit resources to the
Tekton Catalog.

The goals of the TEP are to address the following aspects of the Tekton
Catalog:
- Ownership and Maintenance
- Automated Testing and Dogfooding
- Image Scanning for Common Vulnerabilities and Exposures (CVEs)
- Verified Remote Resources

Related PRs:
- tektoncd#494
- tektoncd#170
- tektoncd#495
jerop added a commit to jerop/community that referenced this pull request Jan 6, 2022
This change adds the problem statement for Support Tiers in the Tekton
Catalog - we will explore the proposal in further changes.

The aim of this TEP is to establish support tiers for resources in the
Tekton Catalog to ensure users get high quality resources that they can
rely on while making it easy for contributors to submit resources to the
Tekton Catalog.

The goals of the TEP are to address the following aspects of the Tekton
Catalog:
- Ownership and Maintenance
- Automated Testing and Dogfooding
- Image Scanning for Common Vulnerabilities and Exposures (CVEs)
- Verified Remote Resources

Related PRs:
- tektoncd#494
- tektoncd#170
- tektoncd#495
jerop added a commit to jerop/community that referenced this pull request Jan 6, 2022
This change adds the problem statement for Support Tiers in the Tekton
Catalog - we will explore the proposal in further changes.

The aim of this TEP is to establish support tiers for resources in the
Tekton Catalog to ensure users get high quality resources that they can
rely on while making it easy for contributors to submit resources to the
Tekton Catalog.

The goals of the TEP are to address the following aspects of the Tekton
Catalog:
- Ownership and Maintenance
- Automated Testing and Dogfooding
- Image Scanning for Common Vulnerabilities and Exposures (CVEs)
- Verified Remote Resources

Related PRs:
- tektoncd#494
- tektoncd#170
- tektoncd#495
tekton-robot pushed a commit that referenced this pull request Jan 6, 2022
This change adds the problem statement for Support Tiers in the Tekton
Catalog - we will explore the proposal in further changes.

The aim of this TEP is to establish support tiers for resources in the
Tekton Catalog to ensure users get high quality resources that they can
rely on while making it easy for contributors to submit resources to the
Tekton Catalog.

The goals of the TEP are to address the following aspects of the Tekton
Catalog:
- Ownership and Maintenance
- Automated Testing and Dogfooding
- Image Scanning for Common Vulnerabilities and Exposures (CVEs)
- Verified Remote Resources

Related PRs:
- #494
- #170
- #495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants