Add a PipelineResourceBase struct that implements some stub versions … #1188
Conversation
googlebot
added
the
cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc 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:
Approvers can indicate their approval by writing |
tekton-robot
added
approved
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
…of the interfaces. This struct can be embedded to simplify and reduce boilerplate across the different resource types, making further refactors easier.
|
The following is the coverage report on pkg/.
|
|
@dlorenc: The following test failed, say
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. |
|
/lgtm |
There was a problem hiding this comment.
I actually want to push back on this a bit: i think inheritance (go says it doesnt have inheritance but i effectively think that's what this is) makes it more difficult to reason about what code is doing - for example, if I want to look at ClusterResource now and understand what it's doing, I have to understand that it is embedding PipelineResourceBase and go look at that definition. Additionally if I'm implementing a new Resource type and I miss a function which I meant to implement, I might not realize it.
My personal feeling is that a slight reduction in the amount of code that needs to be written and faster refactoring are not worth introducing inheritance.
I wonder if there is another way we can tackle this - if we're finding that the interface our resources are implementing contains functions which frequently aren't needed for resources, maybe we should change the interface - maybe we should have a separate object for UploadVolume + DownloadVolume specs for example, with no implementation for types that don't need it.
btw if you are interested, this presentation from pycon 2013 is the one that really convinced me to avoid inheritance at all costs: https://pyvideo.org/pycon-us-2013/the-end-of-object-inheritance-the-beginning-of.html (The End Of Object Inheritance & The Beginning Of A New Modularity by Augie Fackler and Nathaniel Manista)
/hold
tekton-robot
added
the
do-not-merge/hold
I don't think embedding in this fashion is really inheritance: https://medium.com/@simplyianm/why-gos-structs-are-superior-to-class-based-inheritance-b661ba897c67 but don't feel strongly enough about this to argue it. I think that criticism could basically apply to any form of struct or interface embedding, which we (and most Go programs) make heavy use of. |
One idea I was kicking around is to remove most of these methods and give resources one method with a signature something like: func ModifyTaskSpec(spec TaskSpec) (TaskSpec, error)which would be more flexible, but harder to unit test and provide less well-defined lifecycle hooks. Any thoughts on that approach? |
Thanks for the link! Unfortunately I'm still not completely convinced, but I'd really like to be :( Like you said, struct embedding is used all over the place, and I find it makes reasoning about the code very difficult. The article suggests that the main downside of inheritance is that "changing a base class can cause unwanted side effects all over a code base" and I don't see how that's any different when the "base" class is embedded vs. inherited. type Foo struct {}
type Bar struct {
Foo
}
type Baz struct {
Bar
}
func (Foo) A() {
fmt.Println("Foo A")
}
func (Foo) B() {
fmt.Println("Foo B")
}
func (Bar) B() {
fmt.Println("Bar B")
}
func (Baz) C() {
fmt.Println("Baz C")
}
func main() {
f := Baz{}
f.A()
f.B()
f.C()
}Output: Seems nearly as confusing as conventional inheritance to me!
I totally agree that this applies to all embedding, not just this particular case! On a similar note - Python programs use inheritance and polymorphism like crazy, but I'd still maintain that it should be avoided as much as possible.
hmm interesting! this gave me some more ideas (im not super convinced by any of them either tho, and in fact I'm leaning toward going along with what you proposed in the meantime XD - i mention in idea 2 tho that im wondering how this will work with resource extensibility) idea 1 - embedding & modifying TaskSpecWhat do you think about this compromise, which does use struct embedding but I think limits the blast radius a bit more:
What I like about this:
idea 2 - pod specI was trying to think what other parts of the TaskSpec a PipelineResource might need to modify, and I realized it was totally news to me that they needed to add The proposed contract in the resource extensibility proposal has
|
|
/joke |
|
@bobcatfish: Why did the scarecrow win an award? Because he was outstanding in his field. In response to this:
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. |
|
wow |
Now that we don't automatically copy the content of an input to an output (when the same resource is used as both an input and an output), this means that: - Our fan-in / fan-out test will need to explicitly write to the output path, instead of writing to the input path and assuming it would get copied over (the very behaviour we're changing in tektoncd#1188) - Data previously written to an output that is used as an input, and then an output later on, will be lost unless explicitly copied. In the update to the examples this was handled by symlinking the input to the output, I decided to instead update the test to no longer expect to see a file that was written by the first task in the graph and to not copy it explicitly. Note that there is actually a race between the two tasks fanning out - if the were writing the same file we would not be able to reliably predict which would win. Part of fixing tektoncd#1188
Now that we don't automatically copy the content of an input to an output (when the same resource is used as both an input and an output), this means that: - Our fan-in / fan-out test will need to explicitly write to the output path, instead of writing to the input path and assuming it would get copied over (the very behaviour we're changing in #1188) - Data previously written to an output that is used as an input, and then an output later on, will be lost unless explicitly copied. In the update to the examples this was handled by symlinking the input to the output, I decided to instead update the test to no longer expect to see a file that was written by the first task in the graph and to not copy it explicitly. Note that there is actually a race between the two tasks fanning out - if the were writing the same file we would not be able to reliably predict which would win. Part of fixing #1188
…of the interfaces.
Changes
This struct can be embedded to simplify and reduce boilerplate across the different resource
types, making further refactors easier.
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.