Allow PipelineResource implementations to modify the entire Pod spec. #1345
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
|
This is intended to replace #1215 |
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
This change simplifies the interface by removing the GetUpload/Download container and volume methods and replaces it with a more generic "modifier" system. This can be cleaned up a bit more still, and is intended for an early review at this point.
|
The following is the coverage report on pkg/.
|
|
cc @bobcatfish this needs style cleanups - comment fixes and other minor stuff - but can you take another look at the general shape? |
|
Interesting, yeah @dlorenc i think this is pretty close now the the interface we'd have in the extensible version. Interesting that you went the way of having only one func (tm *InternalTaskModifier) GetStepsToPrepend() []Step {
return tm.StepsToPrepend
}
func (tm *InternalTaskModifier) GetStepsToAppend() []Step {
return tm.StepsToAppend
}I think I would have had many Looks good to me! I think this one could probably use a detailed commit message about how this relates to the extensibility model and some of the different iterations we want through fo sho. |
|
/lgtm |
|
lookit all that coverage goin up |
|
|
... and then i merged it so we couldn't. Hey @dlorenc I know it's too late, but would you be open to doing a follow up PR that adds docstrings to the new functions & interface? |
| // resource's data. | ||
| func (s *BuildGCSResource) GetDownloadSteps(sourcePath string) ([]Step, error) { | ||
| // GetInputTaskModifier returns a TaskModifier that prepends a step to a Task to fetch the archive or manifest. | ||
| func (s *BuildGCSResource) GetInputTaskModifier(ts *TaskSpec, sourcePath string) (TaskModifier, error) { |
There was a problem hiding this comment.
im a bit iffy about ts being in this interface, what do you think @dlorenc ? From what I can tell it seems to be for the sake of the getStorageVolumeSpec function, and it looks like that's being used for:
func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec *TaskSpec) ([]corev1.Volume, error) {
var storageVol []corev1.Volume
mountedSecrets := map[string]string{}
for _, volume := range spec.Volumes {
mountedSecrets[volume.Name] = ""
}
...
}
I'm having a hard time understanding what that's actually doing and why it wants access to the TaskSpec to do it
There was a problem hiding this comment.
It looks like it's needed to prevent adding a volume to a task with a name that already exists. I'm not sure if that is actually a realistic scenario, or being overly defensive.
At a minimum, I'll change this to just pass ts as an object rather than a pointer so it can't be mutated.
Nope never!!! Yeah I should have put a /hold on this, sorry :( |
…erged. This adds missing docstrings to the new interface methods and changes the way TaskSpec is passed so it can't be mutated.
This adds missing docstrings to the new interface methods and changes the way TaskSpec is passed so it can't be mutated.
Changes
This change simplifies the interface by removing the GetUpload/Download container
and volume methods and replaces it with a more generic "modifier" system.
This can be cleaned up a bit more still, and is intended for an early review at this point.
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.
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.