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

Only have one set of examples and test that they are valid #109

Merged
merged 4 commits into from
Oct 9, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Only have one set of examples and test that they are valid
We ended up in a state where we had both an `examples` dir and a
`samples` dir, and neither were tested, so both were slightly out of
sync with each other and the actual types.

This was b/c:
- When we used kubebuilder, we generated our types from an original set
  of examples - previously these were in `config/samples` and they ended
  up in `samples`. We used the validation tests generated by kubebuilder
  to test these, but when we removed kubebuilder we removed the tests
- Before presenting the API to the Build CRD working group, we wanted to
  have some more complex, real world examples, which @tejal29 created
  from the `kritis` project and the k8s guestbook example

This commit makes sure that all of the functionality demonstrated in
`samples` is in `examples`, and removes `samples`. This included:
- Fixing `passedConstraints` to be plural as the types expected
- Including references to the `PipelineParams` in the `TaskRun` example
  (removing the duplicated list of result endpoints, since we'll be
  getting this from the references instead)
- Fixing clusterBindings to refer to the actual names of the clusters
  involved

It also adds a step to the integration tests to deploy the examples,
which will fail if they do not match the expected schema. At this point
they are torn down immediately after creation, but in #108 we can expand
this to actually test that they are working.

Also had to make some tweaks to the types:
- actually including `ClusterBindings` in the `Pipeline`
- Using names which include `Resource` instead of `Source`, which we
  moved away from in #39

Fixes #20
bobcatfish committed Oct 9, 2018
commit 91793b5d5ddf8c36435745b156a36d6c74c2be96
9 changes: 6 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
@@ -144,8 +144,11 @@ as inputs and outputs of a `TaskRun`.

* `Resources` is created directly in a pipeline configuration and bound
to `TaskRun` as an input and/or output source.
* The (optional) `passedConstraint` key on an `input source` defines a set of previous task names.
* When the `passedConstraint` key is specified on an input source, only the version of
* The (optional) `passedConstraints` key on an `input source` defines a set of previous task names.
* When the `passedConstraints` key is specified on an input source, only the version of
the resource that passed through the defined list of tasks is used.
* The `passedConstraint` allows for `Tasks` to fan in and fan out, and ordering can be expressed explicitly
* The `passedConstraints` allows for `Tasks` to fan in and fan out, and ordering can be expressed explicitly
using this key since a task needing a resource from a another task would have to run after.

See [docs/pipeline-resources.md](./docs/pipeline-resources.md) for more detail about tyeps
of `Resources`.
4 changes: 2 additions & 2 deletions examples/README.md
Original file line number Diff line number Diff line change
@@ -3,11 +3,11 @@
This directory contains examples of [the Pipeline CRDs](../README.md) in action.

To deploy them to your cluster (after
[installing the CRDs and running the controller](../DEVELOPMENT.md#installing-andrunning)):
[installing the CRDs and running the controller](../DEVELOPMENT.md#getting-started)):

```bash
kubectl apply -f examples/pipelines
kubectl apply -f examples/
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 use ko here ? (like on some other knative projects ?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vdemeester we could but I'm not sure we need to since there's nothing to build - even ko under the hood I think still calls out to kubectl (i think?) - but maybe there are some other benefits I'm missing?

kubectl apply -f examples/pipelines
kubectl apply -f examples/invocations
```

4 changes: 2 additions & 2 deletions examples/deploy_tasks.yaml
Original file line number Diff line number Diff line change
@@ -16,12 +16,12 @@ spec:
- name: image
value: string
cluster:
- name: clusterName
- name: targetCluster
buildSpec:
steps:
- name: deploy
image: kubernetes-helm
args: ['deploy', '--path=${pathToHelmChart}', '--set image=${image}', '${helmArgs}']
args: ['install', '--kube-context=${targetCluster}', '--set image=${image}', '${helmArgs}', '${pathToHelmChart}']

---
apiVersion: pipeline.knative.dev/v1alpha1
28 changes: 8 additions & 20 deletions examples/invocations/run-kritis-test.yaml
Original file line number Diff line number Diff line change
@@ -9,31 +9,19 @@ spec:
name: make
trigger:
triggerRef:
type: manual
type: PipelineRun
name: kritis-pipeline-run-12321312984
pipelineParamsRef:
name: pipelineparams-sample
inputs:
sources:
- name: 'kritis'
type: 'github'
url: 'github.com/grafeas/kritis'
branch: 'featureX'
commit: 'HEAD'
resourcesVersion:
- resourceRef:
name: kritis-resources-git
version: 4da79b91e8e37ea441cfe4972565e2184c1a127f
params:
- name: 'makeTarget'
type: 'string'
Copy link
Member

Choose a reason for hiding this comment

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

Type does not exist here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops, thanks for catching that!

value: 'test'
results:
runs:
name: 'runsBucket'
type: 'gcs'
url: 'gcs://somebucket/results/runs'
logs:
name: 'logBucket'
type: 'gcs'
url: 'gcs://somebucket/results/logs'
tests:
name: 'testBucket'
type: 'gcs'
url: 'gcs://somebucket/results/tests'
status:
steps:
- name: make
18 changes: 9 additions & 9 deletions examples/pipelines/guestbook.yaml
Original file line number Diff line number Diff line change
@@ -45,21 +45,21 @@ spec:
key: workspace
resourceRef:
name: guestbook-resources-git
passedConstraint:
passedConstraints:
- build-guestbook
- build-redis
- name: workspace
key: redis-docker
resourceRef:
name: guestbook-resources-redis-docker
passedConstraint:
passedConstraints:
- build-push
params:
- name: pathToFiles
value: guestbook/all-in-one/guestbook-all-in-one.yaml
clusterBindings:
- inputName: test
key: test
- inputName: clusterName
key: testCluster
- name: int-test-osx # 3.a Run Integration tests for osx
taskRef:
name: integrationTestInDocker
@@ -68,7 +68,7 @@ spec:
key: workspace
resourceRef:
name: guestbook-resources-git
passedConstraint:
passedConstraints:
- deploy-bundle-test
params:
- name: dockerBuildFile
@@ -81,7 +81,7 @@ spec:
key: workspace
resourceRef:
name: guestbook-resources-git
passedConstraint:
passedConstraints:
- deploy-bundle-test
params:
- name: dockerBuildFile
@@ -94,12 +94,12 @@ spec:
key: workspace
resourceRef:
name: guestbook-resources-git
passedConstraint:
passedConstraints:
- int-test-osx
- int-test-linux
params:
- name: pathToFiles
value: guestbook/all-in-one/guestbook-all-in-one.yaml
clusterBindings:
- inputName: staging
key: staging
- inputName: targetCluster
key: stagingCluster
10 changes: 5 additions & 5 deletions examples/pipelines/kritis.yaml
Original file line number Diff line number Diff line change
@@ -24,7 +24,7 @@ spec:
key: workspace # bind to the name in the task
resourceRef:
name: kritis-resources-git
passedConstraint:
passedConstraints:
- unit-test-make
outputSourceBindings:
- name: kritisImage
@@ -42,13 +42,13 @@ spec:
key: workspace
resourceRef:
name: kritis-resources-image
passedConstraint: [build-push]
passedConstraints: [build-push]
params:
- name: pathToHelmCharts
value: kritis-charts
clusterBindings:
- inputName: test
key: test
- inputName: targetCluster
key: testCluster
- name: integration-test # 4. Run Integration Tests in test cluster
taskRef:
name: integration-test-in-docker
@@ -57,7 +57,7 @@ spec:
key: workspace
resourceRef:
name: kritis-resources-test-git
passedConstraint: [deploy-with-helm]
passedConstraints: [deploy-with-helm]
params:
- name: testArgs
value: "-e REMOTE_INTEGRATION=true"
35 changes: 35 additions & 0 deletions examples/smoke-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#!/bin/bash

# Copyright 2018 The Knative Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# smoke_test.sh will attempt to deploy all of the example CRDs to whatever
# is the current kubectl cluster context.
# It will not wait for any Runs to complete and instead will destroy the CRDs
# immediately after creation, so at the moment this pretty much just makes
# sure the structure is valid (and this might even have weird side effects..).

set -o xtrace
set -o errexit
set -o pipefail

kubectl apply -f examples/
kubectl apply -f examples/pipelines
kubectl apply -f examples/invocations

sleep 5

kubectl delete -f examples/invocations
kubectl delete -f examples/pipelines
kubectl delete -f examples/
Copy link
Contributor

Choose a reason for hiding this comment

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

one advantage of using IDE, it adds new line :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i wonder if i need to turn an extension on or something 🤔

3 changes: 3 additions & 0 deletions pkg/apis/pipeline/v1alpha1/pipeline_types.go
Original file line number Diff line number Diff line change
@@ -67,6 +67,9 @@ type PipelineTask struct {
InputSourceBindings []SourceBinding `json:"inputSourceBindings,omitempty"`
// +optional
OutputSourceBindings []SourceBinding `json:"outputSourceBindings,omitempty"`
// TODO(#68) Cluster should become a type of Resource
// +optional
ClusterBindings []ClusterBinding `json:"clusterBindings,omitempty"`
// +optional
Params []Param `json:"params,omitempty"`
}
10 changes: 10 additions & 0 deletions pkg/apis/pipeline/v1alpha1/resource_types.go
Original file line number Diff line number Diff line change
@@ -57,6 +57,16 @@ type PipelineResourceSpec struct {
Params []Param `json:"params"`
}

// TaskResource defines an input or output Resource declared as a requirement
// by a Task. The Name field will be used to refer to these Resources within
// the Task definition, and when provided as an Input, the Name will be the
// path to the volume mounted containing this Resource as an input (e.g.
// an input Resource named `workspace` will be mounted at `/workspace`).
type TaskResource struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Name string `json:"name"`
Type PipelineResourceType `json:"type"`
}

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

19 changes: 3 additions & 16 deletions pkg/apis/pipeline/v1alpha1/task_types.go
Original file line number Diff line number Diff line change
@@ -69,27 +69,14 @@ type Task struct {
// Inputs are the requirements that a task needs to run a Build.
type Inputs struct {
// +optional
Sources []Source `json:"resources,omitempty"`
Resources []TaskResource `json:"resources,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

nice!

// +optional
Params []Param `json:"params,omitempty"`
// TODO(#68) a cluster and/or deployment should be a type of Resource
// +optional
Clusters []Cluster `json:"clusters,omitempty"`
}

// Source is data which is required by a Build/Task for context
// (e.g. a repo from which to build an image). The name of the input will be
// used as the name of the volume containing this context which will be mounted
// into the container executed by the Build/Task, e.g. a Source with the
// name "workspace" would be mounted into "/workspace".
//
// TODO(#62): Something is wrong here, this should be a reference to a resource,
// could just be that the names and comments are out of date.
type Source struct {
// name of the source should match the name of the SourceBinding in the pipeline
Name string `json:"name"`
Type PipelineResourceType `json:"type"`
}

// Param defines arbitrary parameters needed by a task beyond typed inputs
// such as resources.
type Param struct {
@@ -103,7 +90,7 @@ type Outputs struct {
// +optional
Results []TestResult `json:"results,omitempty"`
// +optional
Sources []Source `json:"resources,omitempty"`
Resources []TaskResource `json:"resources,omitempty"`
}

// TestResult allows a task to specify the location where test logs
49 changes: 27 additions & 22 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go
Original file line number Diff line number Diff line change
@@ -87,8 +87,8 @@ func TestAddResourceToBuild(t *testing.T) {
},
Spec: v1alpha1.TaskSpec{
Inputs: &v1alpha1.Inputs{
Sources: []v1alpha1.Source{
v1alpha1.Source{
Resources: []v1alpha1.TaskResource{
v1alpha1.TaskResource{
Name: "workspace",
Type: "git",
},
@@ -288,8 +288,8 @@ func TestAddResourceToBuild(t *testing.T) {
},
Spec: v1alpha1.TaskSpec{
Inputs: &v1alpha1.Inputs{
Sources: []v1alpha1.Source{
v1alpha1.Source{
Resources: []v1alpha1.TaskResource{
v1alpha1.TaskResource{
Name: "workspace-sa",
Type: "git",
},
@@ -336,8 +336,8 @@ func TestAddResourceToBuild(t *testing.T) {
},
Spec: v1alpha1.TaskSpec{
Inputs: &v1alpha1.Inputs{
Sources: []v1alpha1.Source{
v1alpha1.Source{
Resources: []v1alpha1.TaskResource{
v1alpha1.TaskResource{
Name: "workspace-invalid",
Type: "git",
},
Original file line number Diff line number Diff line change
@@ -34,7 +34,7 @@ func AddInputResource(

var gitResource *v1alpha1.GitResource

for _, input := range task.Spec.Inputs.Sources {
for _, input := range task.Spec.Inputs.Resources {
resource, err := pipelineResourceLister.PipelineResources(task.Namespace).Get(input.Name)
if err != nil {
logger.Errorf("Failed to reconcile TaskRun: %q failed to Get Pipeline Resource: %q", task.Name, input.Name)
12 changes: 0 additions & 12 deletions samples/README.md

This file was deleted.

68 changes: 0 additions & 68 deletions samples/pipeline_v1alpha1_pipeline.yaml

This file was deleted.

27 changes: 0 additions & 27 deletions samples/pipeline_v1alpha1_pipelineparams.yaml

This file was deleted.

34 changes: 0 additions & 34 deletions samples/pipeline_v1alpha1_pipelinerun.yaml

This file was deleted.

23 changes: 0 additions & 23 deletions samples/pipeline_v1alpha1_resources.yaml

This file was deleted.

24 changes: 0 additions & 24 deletions samples/pipeline_v1alpha1_task.yaml

This file was deleted.

59 changes: 0 additions & 59 deletions samples/pipeline_v1alpha1_taskrun.yaml

This file was deleted.

9 changes: 8 additions & 1 deletion test/README.md
Original file line number Diff line number Diff line change
@@ -84,10 +84,12 @@ pipelineRunsInformer.Informer().GetIndexer().Add(obj)
## Integration tests

Integration tests live in this directory. To run these tests, you must provide `go` with
`-tags=e2e`, and you may want to use some of the [common flags](#common-flags):
`-tags=e2e`. By default the tests run agains your current kubeconfig context,
but you can change that and other settings with [the flags](#flags):

```shell
go test -v -count=1 -tags=e2e ./test
go test -v -tags=e2e -count=1 ./test --kubeconfig ~/special/kubeconfig --cluster myspecialcluster
```

You can also use
@@ -233,6 +235,11 @@ via the sections for `knative/build-pipeline`.

### Running presubmit integration tests

The presubmit integration tests entrypoint will run:

* [The integration tests](#integration-tests)
* A sanity check deployment of [our example CRDs](../examples)

When run using Prow, integration tests will try to get a new cluster using [boskos](https://github.com/kubernetes/test-infra/tree/master/boskos) and
[these hardcoded GKE projects](https://github.com/knative/test-infra/blob/master/ci/prow/boskos/resources.yaml#L15),
which only [the `knative/test-infra` OWNERS](https://github.com/knative/test-infra/blob/master/OWNERS)
5 changes: 4 additions & 1 deletion test/e2e-tests.sh
Original file line number Diff line number Diff line change
@@ -84,7 +84,10 @@ set +o xtrace
# Wait for pods to be running in the namespaces we are deploying to
wait_until_pods_running knative-build-pipeline || fail_test "Pipeline CRD did not come up"

# Actually run the tests
# Run the smoke tests for the examples dir to make sure they are valid
./examples/smoke-test.sh || fail_test

# Run the integration tests
report_go_test \
-v -tags=e2e -count=1 -timeout=20m ./test \
${options} || fail_test