pkg/pod: extracting function from the package… #4261

vdemeester · 2021-09-27T09:52:13Z

Changes

… to smaller packages.

Signed-off-by: Vincent Demeester vdemeest@redhat.com

/kind cleanup

Opening this for early feedback if those make sense. I am trying to refactor the script and breakpoint part now (both signatures and the way they are tied together and shouldn't probably).

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Refactoring pkg/pod package by splitting it into several smaller more comprehensible packages

tekton-robot · 2021-09-27T09:52:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2021-09-27T09:55:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	87.0%	90.7%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

tekton-robot · 2021-10-04T14:53:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	87.0%	90.7%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

tekton-robot · 2021-10-06T07:21:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	87.0%	90.7%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

tekton-robot · 2021-10-13T08:19:48Z

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

File	Old Coverage	New Coverage
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

tekton-robot · 2021-10-13T08:55:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	86.8%	90.6%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

vdemeester · 2021-10-13T09:27:18Z

/cc @abayer @afrittoli

vdemeester · 2021-10-13T09:32:45Z

/test check-pr-has-kind-label

tekton-robot · 2021-10-14T14:22:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	86.8%	90.6%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

abayer

Looks reasonable - I had a few questions on FIXMEs. =)

abayer · 2021-10-20T15:37:48Z

pkg/pod/entrypoint.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 )

 const (
-	binVolumeName    = "tekton-internal-bin"
-	binDir           = "/tekton/bin"
+	binVolumeName    = "tekton-internal-bin" // FIXME(vdemeester) remove duplication with script package


There comes a time in every project when a pkg/util/const.go ends up being handy. =)

abayer · 2021-10-20T15:48:57Z

pkg/pod/pod.go

@@ -159,7 +163,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
 		return nil, err
 	}

-	// Rewrite steps with entrypoint binary. Append the entrypoint init
+	// Rewrite steps with entrypoint binàary. Append the entrypoint init


Surprise accent! =)

abayer · 2021-10-20T15:54:20Z

pkg/pod/pod.go

@@ -134,18 +135,21 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec

 	// Convert any steps with Script to command+args.
 	// If any are found, append an init container to initialize scripts.
+	// TODO(vdemeester) get one list of steps, return 1 list of containers


Do you mean passing in one list of both steps and sidecars, and getting back one list of both step containers and sidecar containers?

abayer · 2021-10-20T15:58:25Z

pkg/pod/pod.go

@@ -134,18 +135,21 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec

 	// Convert any steps with Script to command+args.
 	// If any are found, append an init container to initialize scripts.
+	// TODO(vdemeester) get one list of steps, return 1 list of containers
+	// TODO(vdemeester) extract breakpoint from scripts


What's the reason to do this? (just trying to understand, since, honestly, last time I was in this code, breakpoints weren't a thing yet!)

tekton-robot · 2021-10-22T10:27:02Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	86.8%	90.6%	3.7
pkg/pod/status/status.go	Do not exist	90.8%

… to smaller packages. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2021-10-25T07:59:57Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/sidecars/sidecars.go	Do not exist	41.2%
pkg/pod/entrypoint.go	86.8%	90.6%	3.7
pkg/pod/script/script.go	Do not exist	100.0%
pkg/pod/status/status.go	Do not exist	90.8%

tekton-robot · 2021-10-25T08:24:55Z

@vdemeester: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-alpha-integration-tests	`1d29739`	link	`/test pull-tekton-pipeline-alpha-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.

tekton-robot · 2021-10-25T08:24:55Z

@vdemeester: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-alpha-integration-tests	`1d29739`	link	`/test pull-tekton-pipeline-alpha-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.

tekton-robot · 2021-10-27T12:13:27Z

@vdemeester: PR needs rebase.

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.

vdemeester · 2021-11-16T09:52:17Z

Rebases are getting out of hand, I'll close this one for now.
/close

tekton-robot · 2021-11-16T09:52:19Z

@vdemeester: Closed this PR.

In response to this:

Rebases are getting out of hand, I'll close this one for now.
/close

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.

tekton-robot requested review from afrittoli and dlorenc September 27, 2021 09:52

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 27, 2021

vdemeester force-pushed the pkg-pod-refactoring branch from 84fbb68 to b54f113 Compare October 4, 2021 14:50

vdemeester force-pushed the pkg-pod-refactoring branch from b54f113 to c3b92ae Compare October 6, 2021 07:17

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2021

vdemeester force-pushed the pkg-pod-refactoring branch from c3b92ae to 4506b13 Compare October 13, 2021 08:16

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2021

vdemeester force-pushed the pkg-pod-refactoring branch from 4506b13 to 1a3c2be Compare October 13, 2021 08:52

vdemeester changed the title ~~wip: pkg/pod: extracting function from the package…~~ pkg/pod: extracting function from the package… Oct 13, 2021

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2021

tekton-robot requested a review from abayer October 13, 2021 09:27

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021

vdemeester force-pushed the pkg-pod-refactoring branch from 1a3c2be to 9d738c0 Compare October 14, 2021 14:20

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021

abayer reviewed Oct 20, 2021

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021

vdemeester force-pushed the pkg-pod-refactoring branch from 9d738c0 to d2e9d01 Compare October 22, 2021 10:23

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2021

pkg/pod: extracting function from the package…

1d29739

… to smaller packages. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester force-pushed the pkg-pod-refactoring branch from d2e9d01 to 1d29739 Compare October 25, 2021 07:57

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2021

tekton-robot closed this Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/pod: extracting function from the package… #4261

pkg/pod: extracting function from the package… #4261

vdemeester commented Sep 27, 2021 •

edited

tekton-robot commented Sep 27, 2021

tekton-robot commented Sep 27, 2021

tekton-robot commented Oct 4, 2021

tekton-robot commented Oct 6, 2021

tekton-robot commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

vdemeester commented Oct 13, 2021

vdemeester commented Oct 13, 2021

tekton-robot commented Oct 14, 2021

abayer left a comment

abayer Oct 20, 2021

abayer Oct 20, 2021

abayer Oct 20, 2021

abayer Oct 20, 2021

tekton-robot commented Oct 22, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 27, 2021

vdemeester commented Nov 16, 2021

tekton-robot commented Nov 16, 2021

pkg/pod: extracting function from the package… #4261

pkg/pod: extracting function from the package… #4261

Conversation

vdemeester commented Sep 27, 2021 • edited

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 27, 2021

tekton-robot commented Sep 27, 2021

tekton-robot commented Oct 4, 2021

tekton-robot commented Oct 6, 2021

tekton-robot commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

vdemeester commented Oct 13, 2021

vdemeester commented Oct 13, 2021

tekton-robot commented Oct 14, 2021

abayer left a comment

Choose a reason for hiding this comment

abayer Oct 20, 2021

Choose a reason for hiding this comment

abayer Oct 20, 2021

Choose a reason for hiding this comment

abayer Oct 20, 2021

Choose a reason for hiding this comment

abayer Oct 20, 2021

Choose a reason for hiding this comment

tekton-robot commented Oct 22, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 25, 2021

tekton-robot commented Oct 27, 2021

vdemeester commented Nov 16, 2021

tekton-robot commented Nov 16, 2021

vdemeester commented Sep 27, 2021 •

edited