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

Add types #2

Merged
merged 6 commits into from
Sep 7, 2018
Merged

Add types #2

merged 6 commits into from
Sep 7, 2018

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Sep 6, 2018

This PR adds the basic Pipeline CRD types from the strawman examples. The types were created from a kubebuilder scaffold. These CRDs include:
Pipeline
PipelineRun
PipelineParams
Task
TaskRun

To install the types into a cluster you can use:
make install

To run the test suite:
make test

For more info on kubebuilder see:
https://book.kubebuilder.io/quick_start.html

@bobcatfish
Copy link
Collaborator

bobcatfish commented Sep 6, 2018

For folks interested in reviewing, the commit that contains the API types specifically is 362223b

The rest of the code is mostly boilerplate built by kubebuilder that allows us to have controllers that can do some (very) basic validation.

Dockerfile Show resolved Hide resolved
cmd/manager/main.go Show resolved Hide resolved
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
creationTimestamp: null
Copy link
Contributor

Choose a reason for hiding this comment

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

can we skip the creationTimestamp filed if its always null?

names:
kind: PipelineParams
plural: pipelineparams
scope: Namespaced
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a design goal that piplines and tasks are Namespace Scoped Vs Cluster Scoped?
If that is the case, we should clarify in the DD or document it somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure we want these to be namespace scoped, we should add this to the README

config/samples/README.md Show resolved Hide resolved
test/controller.go Show resolved Hide resolved
DEVELOPMENT.md Show resolved Hide resolved
// TODO(bobcatfish): when I use `metav1.Time` here I can't figure out
// how to serialize :(
LastTransitionTime string `json:"lastTransitionTime"`

Copy link
Member

Choose a reason for hiding this comment

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

@bobcatfish There are some examples of using metav1.Time in the knative/build repo (https://github.com/knative/build/blob/master/pkg/apis/build/v1alpha1/build_types.go#L193). What issue do you get when you try it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh man! it was a doozy. the error looks like this:

status.conditions.lastTransitionTime in body must be of type object: "string"

This is the result of trying to kubectl apply -f with our samples (e.g. kritis-pipeline-run.yaml), which has content like:

  conditions:
    - type: Started
      status: "True"
      lastTransitionTime: "2018-10-04T12:25:39Z"
      reason: manualTrigger
      message: "Pipeline has been triggered manually"

I couldn't figure out what values to put in for lastTransitionTime that would satisfy the controller when using metav1.Time. It's also a bit odd to do this b/c usually the controller would be populating these values 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I believe it uses the time.RFC3339 format according to this: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/time.go#L114
which looks like this: 2012-11-01T22:08:41+00:00

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @pivotal-nader-ziada , I'll give that a shot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what the issue was prior, changing to metav1.Time appears to be working now. made this change in the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused but happy! I will definitely take it :D

... you know, I wonder if I was using * metav1.Time????

Copy link
Collaborator

Choose a reason for hiding this comment

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

apparently "2018-10-04T12:25:39Z" was an okay format after all! ¯_(ツ)_/¯

Branch string `json:"branch"`
Commit string `json:"commit,omitempty"`
ServiceAccount string `json:"serviceAccount,omitempty"`
}
Copy link
Member

Choose a reason for hiding this comment

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

is this only intended to support git commits or gcs buckets? maybe a generic interface with git and gcs types implementing it?

Copy link
Member

Choose a reason for hiding this comment

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

Also can the Source be the output of the previous task?

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this only intended to support git commits or gcs buckets? maybe a generic interface with git and gcs types implementing it?

Definitely we want to be able to support other sources than just git! It's not clear how exactly to design this though since some tasks might have logic that expects a git commit hash (e.g. naming an image)

I'll add an issue to our issue tracker shortly so we can dive into this more!

Also can the Source be the output of the previous task?

Oh neat! In the current design we didn't allow for that but do you think that's something folks would use?

Copy link
Member

Choose a reason for hiding this comment

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

Can you let us know your thoughts about the other types of Sources and how to support a generic interface?

Yes, I think its useful to for example be able to build an image in a step and run tests on that image just built before uploading it in the following step. This would be need the ability to an input of a task to be a output of a previous task

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you let us know your thoughts about the other types of Sources and how to support a generic interface?

We're not sure yet but I'll tag you when I create an issue to track this investigation!

Yes, I think its useful to for example be able to build an image in a step and run tests on that image just built before uploading it in the following step. This would be need the ability to an input of a task to be a output of a previous task

Ah this is definitely something we support in the current design - however we are assuming the image is pushed to an image repository between steps, which is expensive! you can see what that could look like in this example https://github.com/knative/build-pipeline/blob/master/examples/pipelines/kritis.yaml#L37 where an image built in a previous step is provided as a parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes it harder to make sure the testImage step is actually testing the image that was meant to be created in the buildPush step or picked up an older image because the push failed.

That's a great point. What do you think about being sure that the image is correct by making the digest available? @tejal29 has been suggesting we add an image type that could be an output and an in input - so a Task that builds an image could expose an image, including the digest, and the test Task could be given that image as an input via the Pipeline. (We also want to make sure we make use of the work already done to design build artifacts note doc shared with knative-dev@googlegroups.com).

One downside though is the extra overhead to upload the image to a registry in the build Task and then download it in the test Task :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue of knowing that the sources/inputs you're operating on throughout the pipeline stay the same came up at the build-crd working group meeting also, so maybe we should tackle the issue more generally as well 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree the generic type of input/output might be the way to think about this, an image type can be one implementation, and git another.

We would need to indicate to a taskRun somehow that its input is the output of another taskRun

One downside though is the extra overhead to upload the image to a registry in the build Task and then download it in the test Task :(

Wouldn't we need to do that anyway for the test task to run against the new image?

I think your suggestion of having another issue to cover the design of Source input/output might be the best option to move this forward

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created #11 to discuss particularly the issue of being sure you're using the sources/inputs you expect to be using.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also created #13 to explore sources in more detail!

@bobcatfish bobcatfish added this to Needs review in Pipeline CRD Sep 6, 2018
DEVELOPMENT.md Outdated

You must install these tools:

1. [`go`](https://golang.org/doc/install): The language `Knative Serving` is built in
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Knative serving" should be "Piplien CRD"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@aaron-prindle aaron-prindle merged commit ef113e1 into tektoncd:master Sep 7, 2018
Pipeline CRD automation moved this from Needs review to Done Sep 7, 2018
@bobcatfish bobcatfish mentioned this pull request Sep 7, 2018
bobcatfish referenced this pull request in bobcatfish/pipeline Sep 10, 2018
One of the examples demonstrates building an arbitrary pipeline for
a project called `kritis`, mostly b/c folks who are working on this
project have also worked on `kritis` and are familiar with it, but it's
not actually relevant to `bulid-pipeline` directly. Tried to make this
more clear in the docs.

Also in #2 we had a question about what `strawman` means (not the first
time this has come up!) A defintion is avialable at https://en.wikipedia.org/wiki/Straw_man_proposal
but also mentions it is "american business jargon" so if we want to go
on using that term we should define it. Instead I removed it from the
one place it exists in our docs. Since no one has completely beaten up
the proposal yet it seems to be graduating beyond the "strawman" phase -
but correct me if I'm wrong :D
vdemeester referenced this pull request in vdemeester/tektoncd-pipeline Apr 3, 2019
chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this pull request Apr 11, 2019
chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this pull request Apr 11, 2019
@wlynch wlynch mentioned this pull request Dec 5, 2019
3 tasks
popcor255 added a commit to popcor255/pipeline that referenced this pull request Mar 30, 2020
# This is the 1st commit message:

change tutorial to getting-started

# This is the commit message tektoncd#2:

update some of the docs

# This is the commit message tektoncd#3:

Remove/Modify local registry setup

This is a getting-started guide and if the user was able to get to this doc. It means they have access to the internet. It is probably easier for the user to create docker hub account then to setup a local image registry. It is probably more secure/convenient to push an image to docker hub with an api token that can revoked then depending on a local registry

# This is the commit message tektoncd#4:

change from we to you

chaning we to you because it is the user doing the tutorial

# This is the commit message tektoncd#5:

change from we to you

# This is the commit message tektoncd#6:

parent 6328deb
author popcor255 <popcor255@gmail.com> 1585337853 -0500
committer popcor255 <popcor255@gmail.com> 1585608100 -0400

Restoring tutorial.md

There are alot of docs referencing to this doc. This doc will need to be removed eventually. However, I will keep in here for now

Restoring bullet point that outlines the tutorial

changing 'docker' to Docker

remove 'also' from sentences

change tekton piplines to Tekton Pipelines

changed the logs and regred to have docker.io

changed some of the output logs from the results of the instructions because they did not reflect the changes that were made

correct grammar

The namespace does not matter the tutorial will work either way

Change pipeline

Change pipeline and task with inline-yaml

Remove/Modify local registry setup

This is a getting-started guide and if the user was able to get to this doc. It means they have access to the internet. It is probably user for them to create docker account then to setup a local image registry. It is probably more secure to push an image to docker hub with an api token that can revoked then depending on a local registry

change from we to you

chaning we to you because it is the user doing the tutorial

change from we to you

Restoring tutorial.md

There are alot of docs referencing to this doc. This doc will need to be removed eventually. However, I will keep in here for now

Restoring bullet point that outlines the tutorial

correct grammar

The namespace does not matter the tutorial will work either way

Change pipeline

Change pipeline and task with inline-yaml

# This is the commit message tektoncd#7:

Restoring tutorial.md

There are alot of docs referencing to this doc. This doc will need to be removed eventually. However, I will keep in here for now

# This is the commit message tektoncd#8:

Restoring bullet point that outlines the tutorial
psschwei added a commit to psschwei/pipeline that referenced this pull request Apr 21, 2021
# This is the 1st commit message:

allow dot character in resource names

# The commit message tektoncd#2 will be skipped:

# pipelinerun validation test update
@wlynch wlynch mentioned this pull request Sep 24, 2021
3 tasks
lumjjb pushed a commit to lumjjb/pipeline that referenced this pull request Feb 11, 2022
Signed-off-by: Dan Lorenc <dlorenc@google.com>

changed to use spiffe-csi

Add pod SPIFFE id annotation for workload registrar

Signed-off-by: Brandon Lum <lumjjb@gmail.com>

removed spire jwt

updated obtaining trust bundle

Added SPIFFE entry registration and SVID entrypointer backoff (tektoncd#2)

* Added SPIFFE entry registration and SVID entrypointer backoff

Signed-off-by: Brandon Lum <lumjjb@gmail.com>

* Allow SPIRE configuration through opts

Signed-off-by: Brandon Lum <lumjjb@gmail.com>

* Add validation of SpireConfig

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Yongxuanzhang referenced this pull request in Yongxuanzhang/pipeline May 2, 2022
# This is the 1st commit message:

change emitting results

# This is the commit message #2:

fix code

# This is the commit message #3:

A few minor cleanups in pkg/reconciler/pipelinerun/pipelinerun_test.go

These just mildly annoyed me, so I thought I'd clean them up.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

# This is the commit message #4:

Instrument e2e pipelinerun_test.go files for logstream

These two (`test/pipelinerun_test.go` and `test/v1alpha1/pipelinerun_test.go`)
weren't done in the last PR, because they were messy and I wanted to get that PR
in. But I had some time this morning, so here they are, which should be the last
things in the e2e tests (other than examples/yamls testing, which are their own
bucket of worms) to be instrumented with `helpers.ObjectNameForTest(t)`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

# This is the commit message #5:

Update tutorial links

We recently updated the introductory tutorial on the documnentation
website. That tutorial covers the same content as the one here. To avoid
duplicated efforts and content drift, I'm linkin that doc here and
replacing the existing content.

Additionally, this removes references to `PipelineResources`.

# This is the commit message #6:

Set git-clone tasks in examples to run as root

Similar to tektoncd@ba2e7f3 -
with tektoncd#4758, `git-init` now uses `ghcr.io/distroless/git` as
its base image, and that image needs to run as root. `git-init` by default does _not_ run as root.
So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be
changed to run as root, or the example tests will keep failing forever.

An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using
`git-init:v0.35.0` or later - currently, it's using `v0.29.0`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

# This is the commit message #7:

adding latest release - 0.35

Updating the table to include the latest release links.

# This is the commit message #8:

Added a unit test for pod status.

Add unit test with large result for setTaskRunStatusBasedOnStepStatus.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Pipeline CRD
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants