Skip to content

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented Apr 23, 2020

Changes

One thing that is confusing about Pipelines being beta and
PipelineResources not being beta is that many of our examples still use
PipelineResources.

In an effort to provide more examples of the alternatives, this commit
takes our very first pipeline example, which builds and pushes multiple
images, then deploys services that use the new images, and updates it to
use workspaces and results (and - copies of - catalog tasks) instead of
PipelineResources.

And as a bonus, now that we have results, the last deploy step will use
the digest of the previously built image to ensure it actually deploys
the right image!

In the future, we should add final tasks to this pipeline that ensure
the deployment is actually running; the pipelinerun can succeed even
when the images specified are bogus.

I removed the build-push kaniko taskrun example since we'd probably want to point folks at the kaniko task in the catalog instead.

BONUS: uses volume claim templates as well!! 🎉

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 23, 2020
@bobcatfish
Copy link
Collaborator Author

I0423 21:23:32.131] found examples/v1beta1/pipelineruns/pipelinerun.yaml
I0423 21:23:32.140] found examples/v1beta1/taskruns/build-push-kaniko.yaml
I0423 21:23:33.279] Traceback (most recent call last):
I0423 21:23:33.280]   File "/usr/local/bin/yamllint", line 11, in <module>
I0423 21:23:33.280]     sys.exit(run())
I0423 21:23:33.281]   File "/usr/local/lib/python3.5/dist-packages/yamllint/cli.py", line 181, in run
I0423 21:23:33.281]     problems = linter.run(f, conf, filepath)
I0423 21:23:33.281]   File "/usr/local/lib/python3.5/dist-packages/yamllint/linter.py", line 237, in run
I0423 21:23:33.281]     content = input.read()
I0423 21:23:33.281]   File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode
I0423 21:23:33.282]     return codecs.ascii_decode(input, self.errors)[0]
I0423 21:23:33.282] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 1485: ordinal not in range(128)

whoa

@bobcatfish
Copy link
Collaborator Author

found examples/v1beta1/taskruns/build-push-kaniko.yaml

how did you find that which i have deleted 🤔

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Woohoo!

command:
- echo
args:
- "pass"
Copy link
Member

Choose a reason for hiding this comment

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

Opportunity here to use script mode, and comment that normally this would go test ./... but we don't want this to fail if skaffold pushes a bad change.

Copy link
Member

Choose a reason for hiding this comment

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

by the way, we could use github.com/tektoncd/pipeline here instead, so that we don't depend on anyting but us

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using skaffold might not be as unreliable as we think, since we've tied ourselves to the v0.32.0 release

So maybe longterm the answer is to update it to use our own tests, also tied to a release. :D

And now that we're tied to a revision maybe we can actually run the tests again 🤔

- --destination=$(params.IMAGE)
- --oci-layout-path=$(workspaces.source.path)/$(params.CONTEXT)/image-digest
securityContext:
runAsUser: 0
Copy link
Member

Choose a reason for hiding this comment

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

Comment 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.

This is copied directly from the catalog - I'll copy the comment from the commit message (tektoncd/catalog@0a8b653) and if i'm feeling very ambitious ill open a PR to fix this in catalog also

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020
@vdemeester
Copy link
Member

I0423 21:23:32.131] found examples/v1beta1/pipelineruns/pipelinerun.yaml
I0423 21:23:32.140] found examples/v1beta1/taskruns/build-push-kaniko.yaml
I0423 21:23:33.279] Traceback (most recent call last):
I0423 21:23:33.280]   File "/usr/local/bin/yamllint", line 11, in <module>
I0423 21:23:33.280]     sys.exit(run())
I0423 21:23:33.281]   File "/usr/local/lib/python3.5/dist-packages/yamllint/cli.py", line 181, in run
I0423 21:23:33.281]     problems = linter.run(f, conf, filepath)
I0423 21:23:33.281]   File "/usr/local/lib/python3.5/dist-packages/yamllint/linter.py", line 237, in run
I0423 21:23:33.281]     content = input.read()
I0423 21:23:33.281]   File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode
I0423 21:23:33.282]     return codecs.ascii_decode(input, self.errors)[0]
I0423 21:23:33.282] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 1485: ordinal not in range(128)

whoa

That's weird, I thought we disabled yamllinting in here ? (or was it on catalog 🤔 )

@vdemeester
Copy link
Member

#2485

@vdemeester
Copy link
Member

/test pull-tekton-pipeline-build-tests

1 similar comment
@vdemeester
Copy link
Member

/test pull-tekton-pipeline-build-tests

- name: git-source
volumeClaimTemplate:
metadata:
name: temp-pvc-demo-pipeline-run-1
Copy link
Member

Choose a reason for hiding this comment

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

I liked your proposal about omitting the name of volumeClaimTemplate in #2450

I actually think we should omit the metadata and name part in our examples, it is more readable without and they don't provide much value. It is good to specify metadata when you want to add custom labels or annotations - but that is more advanced usage :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha I left this PR open long enough that I can now use the feature you added @jlpettersson !!! XD thank you!!

One thing that is confusing about Pipelines being beta and
PipelineResources not being beta is that many of our examples still use
PipelineResources.

In an effort to provide more examples of the alternatives, this commit
takes our very first pipeline example, which builds and pushes multiple
images, then deploys services that use the new images, and updates it to
use workspaces and results (and - copies of - catalog tasks) instead of
PipelineResources.

And as a bonus, now that we have results, the last deploy step will use
the digest of the previously built image to ensure it actually deploys
the right image!

In the future, we should add final tasks to this pipeline that ensure
the deployment is actually running; the pipelinerun can succeed even
when the images specified are bogus.

I removed the build-push kaniko taskrun example since we'd probably want
to point folks at the kaniko task in the catalog instead.

BONUS: uses volume claim templates as well!! 🎉
@bobcatfish
Copy link
Collaborator Author

This should be ready for another look!

bobcatfish added a commit to bobcatfish/catalog that referenced this pull request May 11, 2020
I copied this Task (can't wait for OCI registry referencing! :D)
into Pipelines for an example in tektoncd/pipeline#2482
and @imjasonh asked me to add a comment, so I used
tektoncd@0a8b653
to figure out why this was added and now I'm adding the comment
here as well!
bobcatfish added a commit to bobcatfish/catalog that referenced this pull request May 11, 2020
I copied this Task (can't wait for OCI registry referencing! :D)
into Pipelines for an example in tektoncd/pipeline#2482
and @imjasonh asked me to add a comment, so I used
tektoncd@0a8b653
to figure out why this was added and now I'm adding the comment
here as well!
tekton-robot pushed a commit to tektoncd/catalog that referenced this pull request May 11, 2020
I copied this Task (can't wait for OCI registry referencing! :D)
into Pipelines for an example in tektoncd/pipeline#2482
and @imjasonh asked me to add a comment, so I used
0a8b653
to figure out why this was added and now I'm adding the comment
here as well!
@vdemeester
Copy link
Member

/retest

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, 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 [ImJasonH,vdemeester]

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

@ghost
Copy link

ghost commented May 12, 2020

Such beta, very workspace; amaze!

/lgtm

@tekton-robot tekton-robot assigned ghost May 12, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2020
@tekton-robot tekton-robot merged commit 7d164be into tektoncd:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants