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.

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

Seclude general error handler and improve process execution span setup #8726

Merged
merged 9 commits into from Jan 8, 2021

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Jan 7, 2021

Addresses 0.1 and 0.2 from #8364

Additionally fixed tests in two unrelated files as they were immune to side effects of already required utils (ensured there runServerless runs always run with clear require cache)

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #8726 (c278efe) into master (30d89d1) will increase coverage by 0.01%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8726      +/-   ##
==========================================
+ Coverage   87.42%   87.43%   +0.01%     
==========================================
  Files         254      256       +2     
  Lines        9615     9631      +16     
==========================================
+ Hits         8406     8421      +15     
- Misses       1209     1210       +1     
Impacted Files Coverage Δ
lib/classes/Error.js 96.00% <ø> (-2.64%) ⬇️
lib/cli/handle-error.js 98.38% <98.38%> (ø)
lib/Serverless.js 97.19% <100.00%> (-0.13%) ⬇️
lib/cli/resolve-local-serverless-path.js 100.00% <100.00%> (ø)
lib/classes/CLI.js 93.03% <0.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30d89d1...c278efe. Read the comment docs.

@medikoo medikoo force-pushed the 1130-global-error-handling branch 2 times, most recently from 6199d80 to cfd159b Compare January 8, 2021 09:04
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

One comment about duplicated method, otherwise looks great 👍


const consoleLog = (message) => process.stdout.write(`${message}\n`);

const writeMessage = (title, message) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated across two modules or I'm missing the difference between them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I didn't notice (linter simply told me it's still used in Error.js so I didn't touch it).

As I see this logic is also used in logWarning, and it's the reason it's also kept there.

I'm not sure is it worth to clean this up, as (1) logWarning and all other log functions will be removed with #1720 which is also on priority list, and (2) handle-error.js as introduced here is also likely to be improved (in context of this PR we simply take this logic out of Serverless class context, so Serverless is not mangled with CLI specific logic)

It's likely that quite soon writeMessage will be gone from both locations (I also personally don't see it as part of cli utils), and investing time now to make a tested reusable util from it now, doesn't feel that justified.

Let me know

Copy link
Contributor

Choose a reason for hiding this comment

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

If we'll be removing it soon I see zero harm in keeping in duplicated for this short period of time 👍

@medikoo medikoo requested a review from pgrzesik January 8, 2021 13:19
@medikoo medikoo merged commit 847fa34 into master Jan 8, 2021
@medikoo medikoo deleted the 1130-global-error-handling branch January 8, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants