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.

Sign up for GitHub

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

Jump to bottom

Move the workload e2e tests to it's own package #46425

Merged
merged 1 commit into from
Jun 26, 2017
Merged

Move the workload e2e tests to it's own package #46425

merged 1 commit into from
Jun 26, 2017

Conversation

wanghaoran1988
Copy link
Contributor

What this PR does / why we need it:

Move the workload e2e tests to it's own package

Release note:

None

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @wanghaoran1988. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 25, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 25, 2017
@wanghaoran1988 wanghaoran1988 mentioned this pull request May 25, 2017
Closed
@wanghaoran1988 wanghaoran1988 force-pushed the move_workload_e2e_to_own_package branch from 681c626 to bce6769 Compare May 25, 2017 10:08
@wanghaoran1988
Copy link
Contributor Author

/cc @Kargakis @janetkuo @enisoc

@0xmichalis
Copy link
Contributor

@kubernetes/sig-apps-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label May 25, 2017
@0xmichalis
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 25, 2017
@0xmichalis
Copy link
Contributor

Part of #7666

@wanghaoran1988 wanghaoran1988 force-pushed the move_workload_e2e_to_own_package branch 2 times, most recently from ac7e9f3 to 183ebf7 Compare May 25, 2017 15:26
@0xmichalis
Copy link
Contributor

Alongside with this move we need to create a new workload-specific package where all the utilities will live. Currently we have utilities in two different places:
https://github.com/kubernetes/kubernetes/tree/master/test/utils
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/util.go
I'd vote for a test/utils/workloads package.

@wanghaoran1988
Copy link
Contributor Author

@Kargakis Good points, will update tomorrow :)

@wanghaoran1988
Copy link
Contributor Author

@k8s-bot pull-kubernetes-bazel test this

@wanghaoran1988
Copy link
Contributor Author

wanghaoran1988 commented May 26, 2017 •
edited

@Kargakis @ixdy Would you mind tell me why the bazel build failed, and I already run hack/update-bazel.sh .
Error log:
W0526 03:08:46.282] ERROR: /workspace/k8s.io/kubernetes/test/e2e/workload/BUILD:10:1: Target '//pkg/kubectl:go_default_library' is not visible from target '//test/e2e/workload:go_default_library'. Check the visibility declaration of the former target if you think the dependency is legitimate. W0526 03:08:46.520] ERROR: Analysis of target '//test/e2e/workload:go_default_library' failed; build aborted.

@wanghaoran1988
Copy link
Contributor Author

I'd vote for a test/utils/workloads package.

@Kargakis I'd prefer move them to test/e2e/framework, so that other components can also use them conveniently like how we do currently, let me know you have any other concerns.

@0xmichalis
Copy link
Contributor

framework is already bloated; workload-specific utilities should be in their own package. OWNERS files are not valuable when the bot has to pick somebody out of the whole organization.

@fabianofranz
Copy link
Contributor

@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-bazel test this

@wanghaoran1988
Copy link
Contributor Author

@Kargakis I am trying to work as you suggested, but it's hard to split the workloads func from the framework/util.go, there are some basic/common func in framework package that workload related funcs will use, after we split the workload related func to test/utils/workloads, this new package have to depend on the framework package, and some other resource utils under framework package need use the workloads func, this will introduce the cycle import in golang, it's really difficult to split clearly now, so to keep this pr simple, how about address two problems in this pr:

  1. E2e tests move to it's own package
  2. Split every workload related resource utils have it's own util go file, deployment.go rc.go statefu_set.go .etc. this will make it a little easier to maintain currently.

@0xmichalis
Copy link
Contributor

@wanghaoran1988 I am ok with moving just the tests if we need to do more work in order to split the workload-specific utilities.

@wanghaoran1988 wanghaoran1988 force-pushed the move_workload_e2e_to_own_package branch from 183ebf7 to 55a6bc9 Compare June 5, 2017 05:27
@0xmichalis
Copy link
Contributor

@wanghaoran1988 code freeze is lifted, we can merge this as soon as you rebase one more time :)

@wanghaoran1988 wanghaoran1988 force-pushed the move_workload_e2e_to_own_package branch from 025caa5 to 2834fea Compare June 25, 2017 15:51
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 25, 2017
@wanghaoran1988
Copy link
Contributor Author

@Kargakis Cool, updated

@0xmichalis
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2017
@0xmichalis
Copy link
Contributor

/retest

@wanghaoran1988 wanghaoran1988 force-pushed the move_workload_e2e_to_own_package branch from 2834fea to 76251ea Compare June 26, 2017 01:56
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2017
@wanghaoran1988
Copy link
Contributor Author

@Kargakis Hoo, all tests passed now

@0xmichalis
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

2 similar comments
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@0xmichalis
Copy link
Contributor

/retest

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

7 similar comments
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, ixdy, kargakis, wanghaoran1988

Associated issue requirement bypassed by: enisoc

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 49b83c6 into kubernetes:master Jun 26, 2017
@wanghaoran1988 wanghaoran1988 deleted the move_workload_e2e_to_own_package branch June 27, 2017 01:32
@0xmichalis
Copy link
Contributor

Hooray! Thanks a lot @wanghaoran1988 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmichalis 0xmichalis 0xmichalis left review comments

@janetkuo janetkuo Awaiting requested review from janetkuo

@enisoc enisoc Awaiting requested review from enisoc

Assignees

@fabianofranz fabianofranz

@smarterclayton smarterclayton

@0xmichalis 0xmichalis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@wanghaoran1988 @k8s-ci-robot @0xmichalis @fabianofranz @enisoc @ixdy @k8s-github-robot @smarterclayton