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 TaskRun to v1alpha2 🎋 #1725

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Dec 10, 2019

Changes

This modify how TaskRunSpec looks from v1alpha1:

  • params is now directly under spec
  • no more inputs and outputs, get replaced by resources
  • resource has input and output resource declaration fields, similar
    to how it is used in Pipeline

The next step are :

  • Add more types (Pipeline, PipelineRun, Condition)
  • Refactor v1alpha1 to embedded v1alpha2 (for storage purpose)
  • Auto-conversion from v1alpha1

The spec will look like this (only thing that changed from v1alpha1).

spec:
  params:
     # params here
  resources:
    inputs:
      # inputs resources here
    outputs:
      # outputs resource here

One important thing to note here though, is as we are keeping the current PipelineResource as alpha, I can't have embedded ResourceSpec in TaskRun if we keep PipelineResource in the same api group... 😅 😓. (That said, we may be able to do something hacky to bypass that problem 🙈 )

This should more or less fix #1185

Depends on #1706 and #1720 😅

/hold

Signed-off-by: Vincent Demeester [email protected]

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.

Double check this list of stuff that's easy to miss:

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.

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Dec 10, 2019
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 10, 2019
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/contexts.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@bobcatfish
Copy link
Collaborator

One important thing to note here though, is as we are keeping the current PipelineResource as alpha, I can't have embedded ResourceSpec in TaskRun if we keep PipelineResource in the same api group... 😅 sweat. (That said, we may be able to do something hacky to bypass that problem 🙈 )

I think we should try to do something hacky and bypass it so that we can embed ResourceSpecs in TaskRuns even though they are alpha.

// ResourceSpec is specification of a resource that should be created and
// consumed by the task
// +optional
// ResourceSpec *PipelineResourceSpec `json:"resourceSpec,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we move resources e.g. something like pkg/apis/resources/v1alpha1 <-- i think then we'd be able to import resources here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bobcatfish I can see multiple ways indeed 👼 I just need to see how easy or hard it is to bend the generators 👼 😈

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to try that change in a separate PR btw 👼

Copy link
Member Author

Choose a reason for hiding this comment

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

@bobcatfish experiment on this #1773 😅

@vdemeester
Copy link
Member Author

I think we should try to do something hacky and bypass it so that we can embed ResourceSpecs in TaskRuns even though they are alpha.

Ok, will look into that 🙊

limitations under the License.
*/

package v1alpha2
Copy link
Collaborator

Choose a reason for hiding this comment

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

one thing thats not clear to me reviewing: how many of these files and tests are duplicated from v1alpha1 and how many are new? (maybe we can include a docstring or something in the copied files explaining that, maybe in the v1lapha1 version, something like "this file is frozen, changes should now happen in " <-- we could even add automation if we do that to make sure ppl aren't changing files they shouldnt)

Copy link
Collaborator

Choose a reason for hiding this comment

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

bumpin this 😇

@bobcatfish
Copy link
Collaborator

resource has input and output resource declaration fields, similar to how it is used in Pipeline

I think I'm doing a bad job of looking and/or dont know what im looking for: @vdemeester where can I find the logic that will automatically convert v1alpha1 TaskRuns that use the old syntax to the new v1alpha2 syntax?

@vdemeester
Copy link
Member Author

I think I'm doing a bad job of looking and/or dont know what im looking for: @vdemeester where can I find the logic that will automatically convert v1alpha1 TaskRuns that use the old syntax to the new v1alpha2 syntax?

well, nowhere :

The next step are :

    Add more types (Pipeline, PipelineRun, Condition)
    Refactor v1alpha1 to embedded v1alpha2 (for storage purpose)
    Auto-conversion from v1alpha1

In a follow-up 🙇‍♂️

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/contexts.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@vdemeester vdemeester added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@vdemeester
Copy link
Member Author

Rebased but depends on #1773 if we want to support PipelineResource embedded in v1alpha2.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 96.2%

@vdemeester vdemeester removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2020
@vdemeester
Copy link
Member Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@vdemeester
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@vdemeester vdemeester mentioned this pull request Jan 10, 2020
3 tasks
@vdemeester
Copy link
Member Author

/hold

This depends on #1841 too now 👼 😝

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@vdemeester vdemeester force-pushed the v1alpha2-taskrun branch 2 times, most recently from 289b59b to 4a20c66 Compare January 10, 2020 13:19
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/task_validation.go 74.8% 83.1% 8.3
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 97.2%
pkg/apis/pipeline/v1alpha2/workspace_types.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/workspace_validation.go Do not exist 100.0%

@vdemeester
Copy link
Member Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2020
@ghost
Copy link

ghost commented Jan 17, 2020

Oof. Rebase needed.

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2020
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 97.2%

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 97.2%

This modify how `TaskRunSpec` looks from v1alpha1:
- params is now directly under spec
- no more inputs and outputs, get replaced by resources
- resource has input and output resource declaration fields, similar
  to how it is used in Pipeline

The next step are :
- Add more types (Pipeline, PipelineRun, Condition)
- Refactor v1alpha1 to embedded v1alpha2 (for storage purpose)
- Auto-conversion from v1alpha1

Signed-off-by: Vincent Demeester <[email protected]>
@chmouel
Copy link
Member

chmouel commented Jan 20, 2020

/lgtm

Looking Good! 🤙🏽

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2020
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha2/taskrun_defaults.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha2/taskrun_types.go Do not exist 74.1%
pkg/apis/pipeline/v1alpha2/taskrun_validation.go Do not exist 97.2%

@tekton-robot tekton-robot merged commit 99fbc71 into tektoncd:master Jan 20, 2020
@vdemeester vdemeester deleted the v1alpha2-taskrun branch January 20, 2020 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency with how we use and parameterize a taskRef
5 participants