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 common/logger everywhere, remove main/logger export #4317

Closed
wants to merge 1 commit into from
Closed

Use common/logger everywhere, remove main/logger export #4317

wants to merge 1 commit into from

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Nov 10, 2021

  • in Renderer make it 'console' which a stub for silly level

  • in Main make it a winston logger with file output as well

  • in testing make it a jest mock stub

  • in all other cases make it a winston logger without file output

Signed-off-by: Sebastian Malton sebastian@malton.name

@Nokel81 Nokel81 added the chore label Nov 10, 2021
@Nokel81 Nokel81 requested a review from a team as a code owner November 10, 2021 21:39
@Nokel81 Nokel81 requested review from jweak and aleksfront and removed request for a team November 10, 2021 21:39
jakolehm
jakolehm reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@jakolehm jakolehm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove (or split to another PR) all unrelated changes.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Nov 12, 2021

@jakolehm I have undone the changes to download_kubtctl.ts, I think that is the only unrelated change.

jakolehm
jakolehm reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@jakolehm jakolehm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this PR also add no-console eslint rule?

jakolehm
jakolehm reviewed Nov 15, 2021
View reviewed changes
src/common/logger.ts
import { consoleFormat } from "winston-console-format";
import { isDebugging, isTestEnv } from "./vars";
import BrowserConsole from "winston-transport-browserconsole";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify why we are removing winston completely from renderer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, there are several reasons:

  1. Winston does not interact nicely with DevTools and the verbosity selector
  2. "winston-transport-browserconsole" has a bug where any "meta-data" after the second entry is ignored (which I believe is intentional)

src/common/logger.ts Outdated

if (ipcMain) {
transports.push(
if (isTestEnv) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit odd because it relies on a global state... would it be better to have it as a function arg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a function argument to what? I could add a testing function that could be called to mock out the logger, is that what you mean?

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81 Nokel81 mentioned this pull request Nov 18, 2021
Merged
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81 Nokel81 mentioned this pull request Jan 5, 2022
Merged
9 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Nokel81
Use common/logger everywhere, remove main/logger export
9900006 
- in Renderer make it 'console' which a stub for silly level

- in Main make it a winston logger with file output as well

- in testing make it a jest mock stub

- in all other cases make it a winston logger without file output

- Turn on no-console

- Fix logger during integration testing

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Nokel81 Nokel81 marked this pull request as draft January 17, 2022 19:17
@Nokel81
Copy link
Collaborator Author

Nokel81 commented Jan 17, 2022

Closing this as we probably will want two logger injectables in the future.

@Nokel81 Nokel81 closed this Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jweak jweak Awaiting requested review from jweak jweak is a code owner automatically assigned from lensapp/lens-ide

@aleksfront aleksfront Awaiting requested review from aleksfront aleksfront is a code owner automatically assigned from lensapp/lens-ide

@jakolehm jakolehm Awaiting requested review from jakolehm

Assignees
No one assigned
Labels
chore PR: needs rebase
Projects
None yet
Milestone
5.4.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Nokel81 @jakolehm