Removing internal/builder/v1alpha1 and all usages #4270

abayer · 2021-10-01T21:53:11Z

Changes

Part of #3178. internal/builder/v1beta1 is still there - I did remove it from some of the files under pkg/reconciler where I was removing internal/builder/v1alpha1 usage while I was there, but there are still plenty of references left, including 1,487 in pkg/reconciler/pipelinerun/pipelinerun_test.go. Yikes!

/kind cleanup

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

imjasonh · 2021-10-01T22:59:39Z

My hero 😍

abayer · 2021-10-02T00:59:12Z

There’s more to come! I think I may be able to finish off removing everything next week.

abayer · 2021-10-02T15:17:39Z

For future reference, is there a good/easy shortcut to set things up to run the e2e tests locally so I don't have to push and wait 20+ minutes to see why they failed? I swear I had something set up way back in the day, but it's Been Some Time. =)

EDIT: Ok, I found my ancient script to set up my project and cluster in GKE...let's see if that does the trick...yeah, seems good. Is there any way to run against a local k8s cluster, rather than having to run in GKE?

abayer · 2021-10-02T16:19:58Z

Ok, finally got my test cluster working (ko apply -f config/ didn't seem to build/publish/resolve the images, which I could have sworn it used to...dunno if I did something wrong there - @imjasonh, any ideas what I might have borked or if our docs need to be updated?) and fixed what I think is the last failure...

Part of tektoncd#3178. `internal/builder/v1beta1` is still there - I did remove it from some of the files under `pkg/reconciler` where I was removing `internal/builder/v1alpha1` usage while I was there, but there are still plenty of references left, including _1,487_ in `pkg/reconciler/pipelinerun/pipelinerun_test.go`. Yikes! Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2021-10-02T18:20:28Z

/retest

abayer · 2021-10-02T18:54:14Z

fwiw, at my current rate, it might take me up to two weeks to grind out removing all the internal/builder/v1beta1 usages, 'cos pkg/reconciler/taskrun/taskrun_test.go has 1,094 tb.* calls, and pkg/reconciler/pipelinerun/pipelinerun_test.go has 1,487 tb.* calls. Which is...a lot. But I legitimately don't have anything else to do for a while, so dangit, I'm gonna finish it.

vdemeester · 2021-10-04T07:36:31Z

Ok, finally got my test cluster working (ko apply -f config/ didn't seem to build/publish/resolve the images, which I could have sworn it used to...dunno if I did something wrong there - @imjasonh, any ideas what I might have borked or if our docs need to be updated?) and fixed what I think is the last failure...

Check the ko version you have and if it has support for go modules.

pritidesai · 2021-10-04T16:53:53Z

but there are still plenty of references left, including 1,487 in pkg/reconciler/pipelinerun/pipelinerun_test.go. Yikes!

it's a beast 👹 I appreciate all your efforts to cleanup and remove builders 🙏 😻

abayer · 2021-10-04T17:07:10Z

fwiw, this PR is ready to go - I'm planning on getting rid of internal/builder/v1beta1 in a follow-up PR. I'm already about 1/3 of the way through pkg/reconciler/taskrun/taskrun_test.go. =)

imjasonh · 2021-10-04T17:41:09Z

Check the ko version you have and if it has support for go modules.

When in doubt:

upgrade to latest go
upgrade to latest ko
eval $(go env)

This change looks GREAT to me. 😎

/lgtm

abayer · 2021-10-04T21:42:44Z

...ok, looks like I'll have the internal/builder/v1beta1 PR ready by probably sometime Wednesday. I nuked all 1,074 tb.* calls in pkg/reconciler/taskrun/taskrun_test.go in my local branch today, so I've just got 1,487 left in pkg/reconciler/pipelinerun/pipeline_run_test.go.

tekton-robot · 2021-10-05T09:05:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

vdemeester · 2021-10-05T09:05:33Z

/meow

tekton-robot · 2021-10-05T09:05:34Z

@vdemeester:

In response to this:

/meow

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.

pritidesai · 2021-10-05T17:20:51Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go

-					tb.PipelineRunServiceAccountNameTask("task1", "task1SA"),
-					tb.PipelineRunServiceAccountNameTask("task2", "task2SA"),
-				)),
+			pr: &v1alpha1.PipelineRun{


Hey @abayer I appreciate all the cleanup here 🙏 Can I also request to utilize the YAML parser helpers defined here if its possible instead of initiating structs? What do other folks think?

I'm personally not a big fan of embedding YAML in the tests like that, but I'm not opposed to it per se. Lemme finish up removing internal/builder completely and then let's have a follow-up issue for the YAML approach?

sure sounds good, thanks @abayer 👍

(spoiler: I'll probably end up doing the move to YAML approach if there's a consensus on that change, since I've got a good chunk of free time, but I'm so dang close to finishing removing internal/builder that I want to put that to bed first. =) )

tekton-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 1, 2021

tekton-robot requested review from dibyom and a user October 1, 2021 21:53

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 1, 2021

abayer force-pushed the remove-builders-v1alpha1-completely branch from 50c3c72 to 5d3ff17 Compare October 2, 2021 14:13

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 2, 2021

abayer force-pushed the remove-builders-v1alpha1-completely branch 2 times, most recently from 1d52646 to 03e827b Compare October 2, 2021 15:15

abayer force-pushed the remove-builders-v1alpha1-completely branch from 03e827b to bfe27f1 Compare October 2, 2021 16:17

abayer force-pushed the remove-builders-v1alpha1-completely branch from bfe27f1 to 49019b5 Compare October 2, 2021 17:12

tekton-robot assigned imjasonh Oct 4, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2021

vdemeester approved these changes Oct 5, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2021

tekton-robot merged commit 36fd46a into tektoncd:main Oct 5, 2021

jerop mentioned this pull request Oct 5, 2021

Remove condition test builder #4119

Closed

pritidesai reviewed Oct 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing internal/builder/v1alpha1 and all usages #4270

Removing internal/builder/v1alpha1 and all usages #4270

abayer commented Oct 1, 2021 •

edited

Loading

imjasonh commented Oct 1, 2021

abayer commented Oct 2, 2021 via email

abayer commented Oct 2, 2021 •

edited

Loading

abayer commented Oct 2, 2021

abayer commented Oct 2, 2021

abayer commented Oct 2, 2021

vdemeester commented Oct 4, 2021

pritidesai commented Oct 4, 2021

abayer commented Oct 4, 2021

imjasonh commented Oct 4, 2021

abayer commented Oct 4, 2021

tekton-robot commented Oct 5, 2021

vdemeester commented Oct 5, 2021

tekton-robot commented Oct 5, 2021

pritidesai Oct 5, 2021

abayer Oct 5, 2021

pritidesai Oct 5, 2021

abayer Oct 5, 2021

Removing internal/builder/v1alpha1 and all usages #4270

Removing internal/builder/v1alpha1 and all usages #4270

Conversation

abayer commented Oct 1, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

imjasonh commented Oct 1, 2021

abayer commented Oct 2, 2021 via email

abayer commented Oct 2, 2021 • edited Loading

abayer commented Oct 2, 2021

abayer commented Oct 2, 2021

abayer commented Oct 2, 2021

vdemeester commented Oct 4, 2021

pritidesai commented Oct 4, 2021

abayer commented Oct 4, 2021

imjasonh commented Oct 4, 2021

abayer commented Oct 4, 2021

tekton-robot commented Oct 5, 2021

vdemeester commented Oct 5, 2021

tekton-robot commented Oct 5, 2021

pritidesai Oct 5, 2021

Choose a reason for hiding this comment

abayer Oct 5, 2021

Choose a reason for hiding this comment

pritidesai Oct 5, 2021

Choose a reason for hiding this comment

abayer Oct 5, 2021

Choose a reason for hiding this comment

abayer commented Oct 1, 2021 •

edited

Loading

abayer commented Oct 2, 2021 •

edited

Loading