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

bobcatfish · 2018-10-08T16:44:26Z

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 Initial design ideas for Source #39

Fixes #20

bobcatfish · 2018-10-08T17:30:05Z

W1008 16:45:49.586] pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go:37:40: task.Spec.Inputs.Sources undefined (type *"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1".Inputs has no field or method Sources)

oo i broke building!

vdemeester

One comments, but looks good :)
/approve

Needs a rebase though 👼

vdemeester · 2018-10-09T08:09:26Z

examples/README.md


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


Should we use ko here ? (like on some other knative projects ?)

@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?

knative-prow-robot · 2018-10-09T08:10:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

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:

~~OWNERS~~ [bobcatfish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nader-ziada · 2018-10-09T12:58:32Z

examples/invocations/run-kritis-test.yaml

+        resourcesVersion:
+          - resourceRef:
+              name: kritis-resources-git
+            version: 4da79b91e8e37ea441cfe4972565e2184c1a127f
        params:
            - name: 'makeTarget'
              type: 'string'


Type does not exist here

whoops, thanks for catching that!

nader-ziada · 2018-10-09T13:03:04Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -57,27 +57,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"`


nader-ziada

Can we also fix:

examples/pipeline_params.yaml

has a field token that doesn't exist in the type definition

examples/invocations/kritis-pipeline-run.yaml

using prevTask and nextTask that are no longer part of the type definition

bobcatfish · 2018-10-09T18:44:49Z

I think now that #86 is merged I should probably fix the conditions as well 🤔

bobcatfish · 2018-10-09T18:46:05Z

has a field token that doesn't exist in the type definition

good call!

using prevTask and nextTask that are no longer part of the type definition

ah yeah that's right - I think we're still going to need something to represent the order the tasks executed in 🤔 but i'll remove this till we figure it out!

@tejal29

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

None of these fields are in our current spec, so let's remove them for now. As far as `nextTask` and `prevTask` are concerned, we're still going to need some way to represent the order the Tasks actually executed in while the Pipeline was running (maybe? or would we infer this from the Pipeline?) but we can remove them for now.

Now that #95 has gone in, we have better valiation and it revealed a few more issues in our examples. Now the volume mounting example is probably almost right, not sure until we actually try to use it (and implement templating)

We don't yet have validation implemented for our Runs, but as of #86 we are using the same Conditions as Knative Build: just `Succeeded` (where unknown means running, true means it finished successfully and false means it finished but failed)

bobcatfish · 2018-10-09T19:06:56Z

Should be ready for another look @pivotal-nader-ziada @vdemeester !

tejal29 · 2018-10-09T19:57:48Z

examples/smoke-test.sh

+
+kubectl delete -f examples/invocations
+kubectl delete -f examples/pipelines
+kubectl delete -f examples/


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

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

tejal29

/lgtm /hold

tejal29 · 2018-10-09T19:58:26Z

pkg/apis/pipeline/v1alpha1/resource_types.go

+// 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 {


…v0.56.1 feat: upgrade tekton to v0.56.1

knative-prow-robot requested review from imjasonh and nader-ziada October 8, 2018 16:44

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2018

bobcatfish force-pushed the only_samples branch from 62cf6c3 to affcf9a Compare October 8, 2018 17:34

vdemeester approved these changes Oct 9, 2018

View reviewed changes

nader-ziada suggested changes Oct 9, 2018

View reviewed changes

nader-ziada reviewed Oct 9, 2018

View reviewed changes

bobcatfish added 4 commits October 9, 2018 11:50

Fix invalid configuration

ef2e28a

Now that #95 has gone in, we have better valiation and it revealed a few more issues in our examples. Now the volume mounting example is probably almost right, not sure until we actually try to use it (and implement templating)

Update conditions in example runs

739b486

We don't yet have validation implemented for our Runs, but as of #86 we are using the same Conditions as Knative Build: just `Succeeded` (where unknown means running, true means it finished successfully and false means it finished but failed)

bobcatfish force-pushed the only_samples branch from affcf9a to 739b486 Compare October 9, 2018 19:06

knative-prow-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 Oct 9, 2018

tejal29 reviewed Oct 9, 2018

View reviewed changes

tejal29 approved these changes Oct 9, 2018

View reviewed changes

knative-prow-robot assigned tejal29 Oct 9, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2018

knative-prow-robot merged commit 9177893 into tektoncd:master Oct 9, 2018

mchmarny unassigned tejal29 Mar 7, 2019

l-qing added a commit to l-qing/pipeline that referenced this pull request Mar 19, 2024

Merge pull request tektoncd#109 from katanomi/feat/upgrade-tekton-to-…

49a533b

…v0.56.1 feat: upgrade tekton to v0.56.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bobcatfish commented Oct 8, 2018

bobcatfish commented Oct 8, 2018 •

edited

Loading

vdemeester left a comment •

edited

Loading

vdemeester Oct 9, 2018

bobcatfish Oct 9, 2018

knative-prow-robot commented Oct 9, 2018

nader-ziada Oct 9, 2018

bobcatfish Oct 9, 2018

nader-ziada Oct 9, 2018

nader-ziada left a comment •

edited

Loading

bobcatfish commented Oct 9, 2018

bobcatfish commented Oct 9, 2018

bobcatfish commented Oct 9, 2018

tejal29 Oct 9, 2018

bobcatfish Oct 9, 2018

tejal29 left a comment •

edited

Loading

tejal29 Oct 9, 2018

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

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

Conversation

bobcatfish commented Oct 8, 2018

bobcatfish commented Oct 8, 2018 • edited Loading

vdemeester left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada left a comment • edited Loading

Choose a reason for hiding this comment

bobcatfish commented Oct 9, 2018

bobcatfish commented Oct 9, 2018

bobcatfish commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Oct 8, 2018 •

edited

Loading

vdemeester left a comment •

edited

Loading

nader-ziada left a comment •

edited

Loading

tejal29 left a comment •

edited

Loading