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

Do not print logs if print command is used. #5728

Merged
merged 4 commits into from Jan 28, 2019

Conversation

Projects
4 participants
@exoego
Copy link
Contributor

exoego commented Jan 20, 2019

What did you implement:

Closes #5676

How did you implement it:

If print command is used, CLI#log become no-op.
Therefore logs are suppressed, except printing service definition.

Though duplicate-warnings are getting removed in #5733,
I think this change is worth to prevent similar "unexpected log while print" issues in future.

How can we verify it:

  1. npm install -g exoego/serverless#print-no-log
  2. Prepare the below yml.
service: 
  name: myservice
provider:
  name: aws
  runtime: nodejs8.10
functions:
  function1:
    handler: lib/proxy.handler
  function2:
    handler: lib/proxy.handler
  1. sls print, it should only give yml, no warnings.

Todos:

  • Write tests
  • Write documentation N/A
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

exoego added some commits Jan 20, 2019

@pmuens pmuens added the pr/in-review label Jan 21, 2019

@eahefnawy eahefnawy self-assigned this Jan 21, 2019

@eahefnawy eahefnawy self-requested a review Jan 21, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 21, 2019

@eahefnawy eahefnawy moved this from In progress to Needs review in Serverless Jan 21, 2019

@@ -81,6 +81,7 @@ class Serverless {
if (this.cli.displayHelp(this.processedInput)) {
return BbPromise.resolve();
}
this.cli.suppressLog(this.processedInput);

This comment has been minimized.

@eahefnawy

eahefnawy Jan 22, 2019

Member

could you just add a comment here that explains that logs would only be suppressed if the print command is entered? Just to make it clear that logs don't always get suppressed, since all commands will run this method.

or maybe call the method suppressLogIfPrintCommand? 😄

This comment has been minimized.

@exoego

exoego Jan 23, 2019

Author Contributor

Renamed in 59c440e

@eahefnawy
Copy link
Member

eahefnawy left a comment

Beautiful! Thanks @exoego ❤️

Serverless automation moved this from Needs review to Reviewer approved Jan 28, 2019

@eahefnawy eahefnawy merged commit bdea48b into serverless:master Jan 28, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 93.018%
Details

Serverless automation moved this from Reviewer approved to Done Jan 28, 2019

@exoego exoego deleted the exoego:print-no-log branch Jan 28, 2019

@eahefnawy eahefnawy added this to the 1.36.4 milestone Jan 28, 2019

@shortjared shortjared added the bug label Feb 6, 2019

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