[e2e] decouple the aggregated clientset from the e2e testing framework #75989

neolit123 · 2019-04-01T22:44:06Z

What this PR does / why we need it:

Remove usage of the aggregated clientset in the e2e testing framework
itself. We have one test that consumes the clientset in the suite
and it's in test/e2e/apimachinery/aggregator.go, which was recently
promoted to conformance in 8101b86.

This test now obtains a local copy of the aggregated clientset.

Which issue(s) this PR fixes:

xref #75601

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

NONE

/assign @timothysc
/cc @oomichi @andrewsykim
/kind cleanup
/priority important-longterm

neolit123 · 2019-04-02T14:51:20Z

/retest

test/e2e/apimachinery/aggregator.go

neolit123 · 2019-04-09T21:17:55Z

updated
/hold cancel

andrewsykim · 2019-04-09T23:15:51Z

/retest
/lgtm

timothysc

/approve

timothysc · 2019-04-12T16:55:57Z

test/e2e/apimachinery/aggregator.go

+	if err != nil {
+		framework.Failf("could not load config: %v", err)
+	}
+	aggrclient, err := aggregatorclient.NewForConfig(config)


I still think this is weird, but if it's isolated to just this area I'm ok with it.

k8s-ci-robot · 2019-04-12T16:56:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

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:

~~test/OWNERS~~ [timothysc]

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

neolit123 · 2019-04-12T16:58:23Z

/retest

fejta-bot · 2019-04-12T19:29:49Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-04-12T22:17:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

neolit123 · 2019-04-12T23:27:59Z

/test pull-kubernetes-e2e-kind

fejta-bot · 2019-04-13T01:47:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

neolit123 · 2019-04-13T02:07:21Z

/hold

Remove usage of the aggregated clientset in the e2e testing framework itself. We have one test that consumes the clientset in the suite and it's in test/e2e/apimachinery/aggregator.go, which was recently promoted to conformance in 8101b86. This test now obtains a local copy of the aggregated clientset. The suite still has to compile the internal client in. One possible solution here is to move this test in a separate suite, yet it's unclear of how to tackle the problem now that the test has to run as part of the conformance suite.

neolit123 · 2019-04-16T15:06:59Z

test/e2e/apimachinery/aggregator.go

@@ -74,7 +74,17 @@ var _ = SIGDescribe("Aggregator", func() {
 	ginkgo.BeforeEach(func() {
 		c = f.ClientSet
 		ns = f.Namespace.Name
-		aggrclient = f.AggregatorClient
+
+		if aggrclient == nil {


@timothysc i had to do this. ^
otherwise having the LoadConfig()/NewForConfig(config) outside of BeforeEach panics.

k8s-ci-robot · 2019-04-16T15:08:43Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind	a9972934c8528ba3f9dffbe25083004d38e35fa4	link	`/test pull-kubernetes-e2e-kind`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

neolit123 · 2019-04-16T15:14:27Z

/retest

oomichi · 2019-04-16T21:45:05Z

It is nice to move the test specific code from the framework part.

/lgtm

oomichi · 2019-04-16T21:44:17Z

test/e2e/framework/framework.go

@@ -182,8 +180,6 @@ func (f *Framework) BeforeEach() {
 		}
 		f.ClientSet, err = clientset.NewForConfig(config)
 		ExpectNoError(err)
-		f.AggregatorClient, err = aggregatorclient.NewForConfig(config)
-		ExpectNoError(err)
 		f.DynamicClient, err = dynamic.NewForConfig(config)


just one question for next work: I guess we would keep this DynamicClient because the callers are 17 like

test/e2e$ grep f.DynamicClient * -R | wc -l 17

right?

i think we should keep it - this dynamic client is part of client-go.
we only had agreement with api-machinery to move the aggregator.

@neolit123 Thanks for your explanation, that seems reasonable move :-)

neolit123 · 2019-04-16T21:52:37Z

thanks @oomichi
i think there are better solutions for this PR, but given the tests passed:
/hold cancel

optional iterations are welcome.

k8s-ci-robot assigned timothysc Apr 1, 2019

k8s-ci-robot requested review from andrewsykim and oomichi April 1, 2019 22:44

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 1, 2019

neolit123 mentioned this pull request Apr 1, 2019

remove usages of internal clientset in e2e framework #75616

Merged

neolit123 force-pushed the remove-aggregator-from-e2e-fw branch from 6bb13d6 to 6f0b05d Compare April 1, 2019 22:54

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2019

neolit123 force-pushed the remove-aggregator-from-e2e-fw branch from 6f0b05d to 8754cdf Compare April 2, 2019 14:33

neolit123 changed the title ~~[WIP] [e2e] decouple the aggregated clientset from the e2e testing framework~~ [e2e] decouple the aggregated clientset from the e2e testing framework Apr 2, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2019

timothysc reviewed Apr 2, 2019

View reviewed changes

test/e2e/apimachinery/aggregator.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2019

neolit123 force-pushed the remove-aggregator-from-e2e-fw branch 2 times, most recently from 6b4011d to d652f3e Compare April 9, 2019 13:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2019

neolit123 force-pushed the remove-aggregator-from-e2e-fw branch from d652f3e to 751043c Compare April 9, 2019 15:15

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2019

k8s-ci-robot assigned andrewsykim Apr 9, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2019

timothysc approved these changes Apr 12, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2019

neolit123 force-pushed the remove-aggregator-from-e2e-fw branch from a997293 to 760a825 Compare April 16, 2019 15:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2019

neolit123 commented Apr 16, 2019

View reviewed changes

k8s-ci-robot assigned oomichi Apr 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2019

oomichi reviewed Apr 16, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2019

k8s-ci-robot merged commit 92e087c into kubernetes:master Apr 16, 2019

This was referenced Apr 19, 2019

[e2e] Umbrella issue for the e2e testing framework refactor #75601

Closed

[Failing Test] [sig-vmware] [sig-api-machinery] Aggregator Should be able to support the 1.10 Sample API Server using the current Aggregator [Conformance] #75194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[e2e] decouple the aggregated clientset from the e2e testing framework #75989

[e2e] decouple the aggregated clientset from the e2e testing framework #75989

neolit123 commented Apr 1, 2019 •

edited

Loading

neolit123 commented Apr 2, 2019

neolit123 commented Apr 9, 2019

andrewsykim commented Apr 9, 2019

timothysc left a comment

timothysc Apr 12, 2019

k8s-ci-robot commented Apr 12, 2019

neolit123 commented Apr 12, 2019

fejta-bot commented Apr 12, 2019

fejta-bot commented Apr 12, 2019

neolit123 commented Apr 12, 2019

fejta-bot commented Apr 13, 2019

neolit123 commented Apr 13, 2019

neolit123 Apr 16, 2019 •

edited

Loading

k8s-ci-robot commented Apr 16, 2019 •

edited

Loading

neolit123 commented Apr 16, 2019

oomichi commented Apr 16, 2019

oomichi Apr 16, 2019

neolit123 Apr 16, 2019

oomichi Apr 16, 2019

neolit123 commented Apr 16, 2019

[e2e] decouple the aggregated clientset from the e2e testing framework #75989

[e2e] decouple the aggregated clientset from the e2e testing framework #75989

Conversation

neolit123 commented Apr 1, 2019 • edited Loading

neolit123 commented Apr 2, 2019

neolit123 commented Apr 9, 2019

andrewsykim commented Apr 9, 2019

timothysc left a comment

Choose a reason for hiding this comment

timothysc Apr 12, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 12, 2019

neolit123 commented Apr 12, 2019

fejta-bot commented Apr 12, 2019

fejta-bot commented Apr 12, 2019

neolit123 commented Apr 12, 2019

fejta-bot commented Apr 13, 2019

neolit123 commented Apr 13, 2019

neolit123 Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 16, 2019 • edited Loading

neolit123 commented Apr 16, 2019

oomichi commented Apr 16, 2019

oomichi Apr 16, 2019

Choose a reason for hiding this comment

neolit123 Apr 16, 2019

Choose a reason for hiding this comment

oomichi Apr 16, 2019

Choose a reason for hiding this comment

neolit123 commented Apr 16, 2019

neolit123 commented Apr 1, 2019 •

edited

Loading

neolit123 Apr 16, 2019 •

edited

Loading

k8s-ci-robot commented Apr 16, 2019 •

edited

Loading