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

Use log functions of core framework on [r-u] #81685

Merged
merged 1 commit into from Sep 27, 2019
Merged

Use log functions of core framework on [r-u] #81685

merged 1 commit into from Sep 27, 2019

Conversation

oomichi
Copy link
Member

@oomichi oomichi commented Aug 20, 2019 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This makes sub packages of e2elog test framework to use log functions of core
framework instead for avoiding circular dependencies.

Ref: #81427

NOTE: The reason of test/e2e/framework/log/logger_test.go change is the unit test of
logger_test.go checks the stacktrace and the expected stacktrace depends on the line
number of util.go. This PR changes the line number by removing the import of e2elog.
Then this PR needs to reduce the expected line number for passing the unit test.

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2019
@spiffxp
Copy link
Member

spiffxp commented Aug 20, 2019

I'm confused, the PR title seems to indicate switching things to use e2elog, but the code is moving things away from e2elog?

@oomichi oomichi mentioned this pull request Aug 20, 2019
Closed
67 tasks
@oomichi oomichi changed the title Switch to use e2elog of core framework on [r-u] Switch to use log functions of the core framework on [r-u] Aug 20, 2019
@oomichi
Copy link
Member Author

oomichi commented Aug 20, 2019 •
edited

@spiffxp

I'm confused, the PR title seems to indicate switching things to use e2elog, but the code is moving things away from e2elog?

Yeah, you are right.
And I see the original message made confusions easily, so I've updated the messages of this PR, the commit message and the corresponding issue.
Sorry about that.

@oomichi oomichi force-pushed the e2elog-framework-p-r branch 2 times, most recently from 5541c9e to 0fc89c6 Compare August 20, 2019 21:57
@oomichi oomichi changed the title Switch to use log functions of the core framework on [r-u] Use log functions of core framework on [r-u] Aug 21, 2019
@spiffxp
Copy link
Member

spiffxp commented Aug 21, 2019

/cc @andrewsykim
I've tried following along with the linked slack discussion and such and it's still not 100% clear to me whether consensus was reached and this was the path agreed upon, can you take a look?

@oomichi oomichi force-pushed the e2elog-framework-p-r branch 4 times, most recently from 75a57b3 to 857e357 Compare August 28, 2019 01:33
@oomichi
Copy link
Member Author

oomichi commented Aug 28, 2019

/retest

@oomichi
Copy link
Member Author

oomichi commented Aug 28, 2019

/retest

1 similar comment
@oomichi
Copy link
Member Author

oomichi commented Aug 28, 2019

/retest

@oomichi oomichi mentioned this pull request Aug 28, 2019
Merged
@timothysc
Copy link
Member

@oomichi this is the exact opposite of what we are trying todo in breaking apart the framework.

closing.

@timothysc timothysc closed this Sep 10, 2019
@timothysc timothysc reopened this Sep 11, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2019
@timothysc timothysc added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Sep 20, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 20, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2019
@oomichi oomichi force-pushed the e2elog-framework-p-r branch 2 times, most recently from c027c43 to cf9cc73 Compare September 25, 2019 23:43
@oomichi
Copy link
Member Author

oomichi commented Sep 25, 2019

/test pull-kubernetes-verify

@oomichi
Copy link
Member Author

oomichi commented Sep 26, 2019

/test pull-kubernetes-conformance-kind-ipv6

@s-ito-ts
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 Sep 26, 2019
@fejta-bot
Copy link

/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.

2 similar comments
@fejta-bot
Copy link

/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
Copy link

/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.

Use log functions of core framework on [r-u]
6499f93 
This makes sub packages of e2e test framework to use log functions
of core framework instead for avoiding circular dependencies.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2019
@s-ito-ts
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 Sep 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit d92a250 into kubernetes:master Sep 27, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiffxp spiffxp Awaiting requested review from spiffxp

@timothysc timothysc Awaiting requested review from timothysc

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

Assignees

@s-ito-ts s-ito-ts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@oomichi @k8s-ci-robot @spiffxp @timothysc @s-ito-ts @fejta-bot