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

Modern logs for standalone-specific commands #10034

Merged
merged 2 commits into from Oct 4, 2021
Merged

Modern logs for standalone-specific commands #10034

merged 2 commits into from Oct 4, 2021

Conversation

pgrzesik
Copy link
Contributor

@pgrzesik pgrzesik commented Oct 1, 2021

Addresses: #9860

@pgrzesik pgrzesik self-assigned this Oct 1, 2021
@codecov
Copy link

codecov bot commented Oct 1, 2021 •
edited

Codecov Report

Merging #10034 (cd525c3) into master (f73062f) will decrease coverage by 0.18%.
The diff coverage is 74.46%.

❗ Current head cd525c3 differs from pull request most recent head 6e865e7. Consider uploading reports for the commit 6e865e7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10034      +/-   ##
==========================================
- Coverage   85.88%   85.70%   -0.19%     
==========================================
  Files         333      333              
  Lines       13416    13434      +18     
==========================================
- Hits        11523    11514       -9     
- Misses       1893     1920      +27
Impacted Files Coverage Δ
lib/cli/write-service-outputs.js 83.33% <ø> (-2.39%) ⬇️
lib/plugins/standalone.js 41.50% <14.28%> (-5.01%) ⬇️
lib/cli/handle-error.js 85.71% <100.00%> (-0.29%) ⬇️
lib/plugins/aws/deploy/index.js 100.00% <100.00%> (ø)
lib/plugins/aws/info/index.js 100.00% <100.00%> (ø)
lib/plugins/aws/logs.js 87.71% <100.00%> (-0.42%) ⬇️
lib/plugins/aws/utils/formatLambdaLogEvent.js 100.00% <100.00%> (ø)
lib/plugins/install.js 100.00% <100.00%> (ø)
lib/plugins/print.js 71.21% <100.00%> (-28.79%) ⬇️
lib/utils/downloadTemplateFromRepo.js 96.77% <100.00%> (ø)
... and 1 more

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 f73062f...6e865e7. Read the comment docs.

@pgrzesik pgrzesik requested a review from medikoo October 1, 2021 13:59
medikoo
medikoo reviewed Oct 1, 2021
View reviewed changes
lib/plugins/standalone.js Outdated
const tagName = await standaloneUtils.resolveLatestTag();
const latestVersion = tagName.slice(1);
if (latestVersion === currentVersion) {
this.serverless.cli.log('Already at latest version');
const message = 'Already at latest version';
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't introduce those variables, as when we will remove the legacy calls, they will leave unnecessary verbosity.

I think it's fine in this case to keep given messages mentioned twice, it'll pay off in v3 branch

@pgrzesik pgrzesik requested a review from medikoo October 4, 2021 09:28
medikoo
medikoo reviewed Oct 4, 2021
View reviewed changes
lib/plugins/standalone.js Outdated
@@ -40,10 +42,17 @@ module.exports = class Standalone {
}

async upgrade() {
const commandRunStartTime = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can rely on pluginManager.commandRunStartTime (?)

@pgrzesik pgrzesik requested a review from medikoo October 4, 2021 15:30
medikoo
medikoo approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@pgrzesik pgrzesik merged commit 9b5e6b1 into master Oct 4, 2021
@pgrzesik pgrzesik deleted the modern-logs-for-standalone branch October 4, 2021 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medikoo medikoo medikoo approved these changes

Assignees

@pgrzesik pgrzesik

Labels
cat/cli refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pgrzesik @medikoo