Use portforwarder instead of kubectl command line #599

NavidZ · 2020-06-08T15:58:38Z

Changes

Make use of the portforward object instead of calling
into kubectl via command line exec.

This will remove the need for random sleep amount after
running the kubectl until port forwarding is ready and
make it more reliable.

Fixes #496

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

linux-foundation-easycla · 2020-06-08T15:58:41Z

The committers are authorized under a signed CLA.

✅ Navid Zolghadr (f9096d6)

tekton-robot · 2020-06-08T15:58:53Z

Hi @NavidZ. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

NavidZ · 2020-06-08T16:13:04Z

I wasn't sure whether I should have included changes beyond the changes in eventlistener_test.go file. All those are the result of vendor update that this change needs to get newer version of k8s.io/client-go.

dibyom · 2020-06-08T16:37:26Z

/ok-to-test

NavidZ · 2020-06-08T16:46:15Z

test/eventlistener_test.go

+
+	config, err := clientcmd.BuildConfigFromFlags("", knativetest.Flags.Kubeconfig)
+	if err != nil {
+		panic(err)


I just realized these "panic"s slipped through. I'll update them in the next patch.

NavidZ · 2020-06-12T19:20:45Z

/assign wlynch

NavidZ · 2020-06-12T19:39:53Z

/kind cleanup

NavidZ · 2020-06-12T19:49:55Z

/kind flake

dibyom · 2020-06-15T14:28:41Z

test/eventlistener_test.go

+		}
+
+		path := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s/portforward", namespace, podName)
+		hostIP := strings.TrimLeft(config.Host, "htps:/")


is the htps:/ supposed to be https:/?

It seem TrimLeft just gets a set of characters and remove all the ones from the beginning of the string. I can use the other function that only removes the exact keyword if you prefer.

yeah, I would use strings.TrimPrefix instead, or call url.Parse and then set the path afterwards.

dibyom · 2020-06-15T14:33:48Z

Would you mind squashing up the commits? Thanks!

NavidZ · 2020-06-15T14:40:22Z

Would you mind squashing up the commits? Thanks!

I had a question here. I also noticed this in other reviews. GitHub already has a squash button before merging when doing a manual merge but I guess since the bot merges these after an lgtm command it doesn't do it automatically.
So regarding doing this locally, is that alright to always do it? In other words do people in Tekton get upset if the history is lost between the comments in general (i.e. if I always just apply comments with a commit --am and force push it on this PR) or should I keep committing on top right until before merge and do one squash at the end?

wlynch · 2020-06-15T15:13:57Z

test/eventlistener_test.go

+		}
+
+		path := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s/portforward", namespace, podName)
+		hostIP := strings.TrimLeft(config.Host, "htps:/")


yeah, I would use strings.TrimPrefix instead, or call url.Parse and then set the path afterwards.

wlynch · 2020-06-15T15:19:40Z

test/eventlistener_test.go

-	// Wait for port forward to take effect
-	time.Sleep(5 * time.Second)
+	go func(stopChan chan struct{}, errChan chan error) {
+		readyChan := make(chan struct{}, 1)


nit: move this below so that it's closer to where it's being used.

Make use of the portforward object instead of calling into kubectl via command line exec. This will remove the need for random sleep amount after running the kubectl until port forwarding is ready and make it more reliable.

dibyom · 2020-06-15T16:30:43Z

So regarding doing this locally, is that alright to always do it? In other words do people in Tekton get upset if the history is lost between the comments in general (i.e. if I always just apply comments with a commit --am and force push it on this PR) or should I keep committing on top right until before merge and do one squash at the end?

We don't have a clear rule on this. For larger PRs, I do one squash at the end, for smaller ones I just keep ammending and pushing. I don't think anyone would be upset either way 😄

NavidZ · 2020-06-16T15:35:55Z

/assign iancoffey

iancoffey

lgtm 👍

tekton-robot · 2020-06-16T16:13:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, iancoffey

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~~ [dibyom,iancoffey]

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

dibyom · 2020-06-16T17:15:26Z

/lgtm

tekton-robot requested review from iancoffey and wlynch June 8, 2020 15:58

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 8, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 8, 2020

NavidZ commented Jun 8, 2020

View reviewed changes

tekton-robot assigned wlynch Jun 12, 2020

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jun 12, 2020

tekton-robot added the kind/flake Categorizes issue or PR as related to a flakey test label Jun 12, 2020

dibyom reviewed Jun 15, 2020

View reviewed changes

wlynch reviewed Jun 15, 2020

View reviewed changes

Use portforwarder instead of kubectl command line

2b64df5

Make use of the portforward object instead of calling into kubectl via command line exec. This will remove the need for random sleep amount after running the kubectl until port forwarding is ready and make it more reliable.

NavidZ force-pushed the improve-eventlistener_test branch from 1b01383 to 2b64df5 Compare June 15, 2020 16:29

dibyom approved these changes Jun 16, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2020

tekton-robot assigned iancoffey Jun 16, 2020

iancoffey approved these changes Jun 16, 2020

View reviewed changes

tekton-robot assigned dibyom Jun 16, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020

tekton-robot merged commit 9dcc13c into tektoncd:master Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use portforwarder instead of kubectl command line #599

Use portforwarder instead of kubectl command line #599

NavidZ commented Jun 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading

tekton-robot commented Jun 8, 2020

NavidZ commented Jun 8, 2020

dibyom commented Jun 8, 2020

NavidZ Jun 8, 2020

NavidZ commented Jun 12, 2020

NavidZ commented Jun 12, 2020

NavidZ commented Jun 12, 2020

dibyom Jun 15, 2020

NavidZ Jun 15, 2020

wlynch Jun 15, 2020

dibyom commented Jun 15, 2020

NavidZ commented Jun 15, 2020

wlynch Jun 15, 2020

wlynch Jun 15, 2020

dibyom commented Jun 15, 2020

NavidZ commented Jun 16, 2020

iancoffey left a comment

tekton-robot commented Jun 16, 2020

dibyom commented Jun 16, 2020

Use portforwarder instead of kubectl command line #599

Use portforwarder instead of kubectl command line #599

Conversation

NavidZ commented Jun 8, 2020 • edited Loading

Changes

Submitter Checklist

linux-foundation-easycla bot commented Jun 8, 2020 • edited Loading

tekton-robot commented Jun 8, 2020

NavidZ commented Jun 8, 2020

dibyom commented Jun 8, 2020

NavidZ Jun 8, 2020

Choose a reason for hiding this comment

NavidZ commented Jun 12, 2020

NavidZ commented Jun 12, 2020

NavidZ commented Jun 12, 2020

dibyom Jun 15, 2020

Choose a reason for hiding this comment

NavidZ Jun 15, 2020

Choose a reason for hiding this comment

wlynch Jun 15, 2020

Choose a reason for hiding this comment

dibyom commented Jun 15, 2020

NavidZ commented Jun 15, 2020

wlynch Jun 15, 2020

Choose a reason for hiding this comment

wlynch Jun 15, 2020

Choose a reason for hiding this comment

dibyom commented Jun 15, 2020

NavidZ commented Jun 16, 2020

iancoffey left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 16, 2020

dibyom commented Jun 16, 2020

NavidZ commented Jun 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading