Remove PipelineParams

[nader-ziada]

The goal of this PR is to start a discussion about removing PipelineParams and moving its contents to PipelineRun.

In case we decide we want to do that, this is what things would look like

Proposed changes

• Move ServiceAccount and Results to PipelineRun

Fixes: #121

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada

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 [pivotal-nader-ziada]

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

[nader-ziada]

/cc @bobcatfish
What do you think?

[nader-ziada]

/hold

until we discuss and decide what we want to do

[bobcatfish]

Exciting! I'm defintiely all for moving the service account

I'm not sure about the results tho - i think it depends on how frequently we expect this to change. my preference would be to rename pipelineparams to something like ResultsConfig and leave the results stuff there - @tejal29 will probably have some opinions about how to handle this when she's back but @aaron-prindle might be able to weigh in as well

p.s. this should address #121 can i assign that one to you @pivotal-nader-ziada ? :D

[nader-ziada]

I was thinking the results would be part of the runtime (pipelineRun) because you would change it to run in your own environment without having to change the original pipeline. You might also need to change it with every run if you want to keep history depending on how it will be implemented.
Also I prefer to have less files to deal with as a user building/running a pipeline, having a file for the results location only seems like something that should be simplified

@bobcatfish yes assign me the issue :)

[bobcatfish]

while we're at it, why don't we simplify this and get rid of Tests and Runs? Discussions with @tejal29 indicated that we might treat Tests like a type of resource instead, and it's not clear how we'll use Runs

(sorry for the scope creep!!)

[bobcatfish]

You might also need to change it with every run if you want to keep history depending on how it will be implemented.

I guess we have to see how #107 plays out!

I was assuming that the log config wouldn't change as often as some of the other Resources (e.g. git revisions) b/c you'd provide your bucket and we'd make sure we created separate dirs for each Run, etc. Maybe it'll be too hard to make that generic tho 🤔

Okay let's go ahead with this for now - we can always add back a new CRD for results if we need it for #17

[bobcatfish]

/lgtm

[bobcatfish]

/meow space

[knative-prow-robot]

@bobcatfish:

In response to this:

/meow space

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.

[bobcatfish]

(feel free to remove the hold if you don't feel like removing Tests and Runs from results in this iteration)

[nader-ziada]

/hold cancel

@bobcatfish made the changes to remove Runs and Tests, we can always add them again when we figure out more details

Please take another look

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/controller/main.go	3.5%	3.6%	0.1
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	100.0%	88.0%	-12.0
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	93.3%	94.6%	1.3

[bobcatfish]

Thanks @pivotal-nader-ziada !

/lgtm

So long paramses!