Change TaskRef to string instead of a struct with just one field.

[tejal29]

Addresses one of the redundancy for #138
Right now, the TaskRef struct only declares field Name.
Defining a struct for a just 1 field makes yaml longer.
In attempt to make the pipleine config concise, this PR remoes TaskRef and replaces it with a string.
In future if we decide to add other fields like APIVersion, we can write reference it as ":".
Right now APIVersion is not declared and used at all, so removing it.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tejal29

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 [tejal29]

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

[knative-prow-robot]

@tejal29: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-build-pipeline-unit-tests	6f32d0c	link	/test pull-knative-build-pipeline-unit-tests
pull-knative-build-pipeline-integration-tests	6f32d0c	link	/test pull-knative-build-pipeline-integration-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[bobcatfish]

I'm not sure we should go ahead with this - it does simplify the yaml but it also:

1. Isn't the format a k8s reference is supposed to be in (not sure if that's important or not!)
2. Will get in the way of duck typing later on, for which we would need to in fact add at least a type field to the reference (more important probably)

I don't feel like I'm even approaching being an expert on this topic tho so maybe @dlorenc @imjasonh @jonjohnsonjr could chime in?

Re. the kubernetes conventions:

References to specific objects, especially specific resource versions and/or specific fields of those objects, are specified using the ObjectReference type (or other types representing strict subsets of it).

[bobcatfish]

/hold

[shashwathi]

@tejal29 :

Object references are made in a separate struct because it needs API version and reference name. In TaskRef struct looks like API Version is missing, I would recommend adding that to struct and hold on to current yaml structure instead of removing the reference struct.

Ref: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#objectreference-v1-core

If this change is implemented then you cannot refer to an object with different API version, I agree there are no multiple versions now but in future it might be the case.
WDYT?

[nader-ziada]

agree with @shashwathi and @bobcatfish, if we remove the struct, then we can't have an API version in the future

[bobcatfish]

kk sounds like we probably want to leave this as it is for now then. @tejal29 you also mentioned possibly encoding the version (and I guess eventually type) in the name field as well, but I think that makes this more complicated in the long run - we have to start requiring strings of a certain format and parsing strings vs. trusting the fields

[tejal29]

How about we call it taskName https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#object-references

Object references should either be called fooName if referring to an object of kind Foo by just the name (within the current namespace, if a namespaced resource), or should be called fooRef, and should contain a subset of the fields of the ObjectReference type.

@shashwathi, @bobcatfish and@pivotal-nader-ziada, how are we going to support Task with different versions?
They would probably belong to ObjectMeta but most of the fields there are read only
Adding a version in spec would not solve the purpose unless they belong to different api grps?
Why not use something which is more user friendly now?

[bobcatfish]

how are we going to support Task with different versions?

I'm not sure about versions, but probably different types in the long run, as per knative duck typing.

Why not use something which is more user friendly now?

It doesn't seem significantly more user friendly - we're talking about one level of indentation:

The cost would be that if we change this later, we need to either support both (confusing for a user) or introduce a breaking change (bad for a user).

I'd rather stick with the k8s standard for object references.

[tejal29]

hmm but fooName is Kubernetes Object Reference std

[bobcatfish]

hmm but fooName is Kubernetes Object Reference std

Oh I see what you're saying, fooName is better than fooRef if we flatten this.

So then I think it comes down to two questions:

1. Do we do this for all our references? I'd prefer to be consistent if we can
2. How likely we think we are to want to support different kinds of Types for those references?

If the answer to (2) is not very likely, then let's change all the fooRefs to fooName! My gut feeling is that this is limiting our options in the future with the API, but it's definitely simpler now.

Maybe we can see what @shashwathi or @pivotal-nader-ziada think now as a tie breaker.

[nader-ziada]

I have seen both standards used in kubernetes and other knative projects, using object ref allows for more flexibility which I prefer.
I can see the answer to question 2 above is possibly we can use a taskrun as the execution engine to fetch resources or send notifications.
tldr, I prefer object ref for flexibility, but can live with the text field if there is strong preference for it.

[bobcatfish]

@tejal29 do you mind if we close this for now?

[tejal29]

Disregarding.