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

[WIP] Adds support for v1 versions of Tekton #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PuneetPunamiya
Copy link
Member

No description provided.

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from puneetpunamiya after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 10, 2024
@PuneetPunamiya PuneetPunamiya changed the title Adds support for v1 versions of Tekton [WIP] Adds support for v1 versions of Tekton Jun 10, 2024
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2024
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-catlin-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/linter/script.go 68.0% 66.7% -1.3
pkg/parser/parser.go 69.2% 0.0% -69.2
pkg/parser/parser.go Do not exist 68.6%

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-catlin-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/linter/script.go 68.0% 66.7% -1.3
pkg/parser/parser.go 69.2% 0.0% -69.2
pkg/parser/parser.go Do not exist 68.6%

@@ -22,7 +22,7 @@ import (

"github.com/tektoncd/catlin/pkg/parser"
"github.com/tektoncd/catlin/pkg/validator"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"

Comment on lines +172 to +174
// case "clustertask":
// task := res.(*v1beta1.ClusterTask)
// t.collectOverSteps(task.Spec.Steps, task.ObjectMeta.Name, &result)
Copy link
Member

Choose a reason for hiding this comment

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

let's remove this?

Comment on lines +62 to +74
// const clusterTaskTest = `
// apiVersion: tekton.dev/v1beta1
// kind: ClusterTask
// metadata:
// name: hello-moto
// spec:
// steps:
// - name: nogood
// image: image1
// script: |
// #!/usr/bin/env sh
// '
// `
Copy link
Member

Choose a reason for hiding this comment

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

same

Comment on lines +144 to +157
// func Test_ClusterTaskParse(t *testing.T) {
// r := strings.NewReader(clusterTaskTest)
// parser := parser.ForReader(r)

res, err := parser.Parse()
assert.NilError(t, err)
// res, err := parser.Parse()
// assert.NilError(t, err)

tl := &taskLinter{
res: res,
configs: configSh,
}
result := tl.Validate()
assert.Equal(t, 1, result.Errors)
}
// tl := &taskLinter{
// res: res,
// configs: configSh,
// }
// result := tl.Validate()
// assert.Equal(t, 1, result.Errors)
// }
Copy link
Member

Choose a reason for hiding this comment

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

same or let's add t.Skip() if we need to keep it

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
)

func registerSchema() {
beta1 := runtime.NewSchemeBuilder(v1beta1.AddToScheme)
beta1 := runtime.NewSchemeBuilder(v1beta1.AddToScheme, v1.AddToScheme)
Copy link
Member

Choose a reason for hiding this comment

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

let's rename the variable from beta1 to maybe builder

@@ -154,7 +155,7 @@ type tektonResource interface {
func typeForKind(kind string) (tektonResource, error) {
switch kind {
case "Task":
return &v1beta1.Task{}, nil
return &v1.Task{}, nil
Copy link
Member

Choose a reason for hiding this comment

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

should we add tests for both v1 and v1beta1 to ensure things are working for both API version ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants