miguel-a-calles-mba / serverless-stack-termination-protection

Serverless plugin to update the CloudFormation stack termination protection.
MIT License
23 stars 4 forks source link

feat: Update console logging for Serverless Framework v3 #22

Closed coyoteecd closed 2 years ago

coyoteecd commented 2 years ago

@miguel-a-calles-mba

With the release of v3, the Serverless folks have redesigned the CLI logging output for plugins. I have a few plugins I maintain and updated them to support v3 only, using the plugin guidelines available at https://www.serverless.com/framework/docs/guides/plugins/cli-output.

Since I use serverless-termination-protection plugin as well, I'd like to update this one to use the new logging API. For now, it is purely for cosmetic reasons, however moving forward I'd expect the deprecated cli.log function will be removed. Example of deploy output:

image

Would you merge a PR that breaks backwards compatibility (so major release) that uses the new logging API exclusively? It can be done in backwards-compatible manner too, but will require log wrappers for log.success/log.error

miguel-a-calles-mba commented 2 years ago

Hi @coyoteecd, sure thing.

Would you mind testing PR #23 for me, please?

npm i miguel-a-calles-mba/serverless-stack-termination-protection#pull/23

Thanks!

coyoteecd commented 2 years ago

@miguel-a-calles-mba sorry for the delay.

I tested the plugin today. While it does work, the CLI output does not follow the guidelines of Serverless folks, namely:

For the first one, see also this post in a conversation I had a while ago related to a plugin that I've been updating: https://github.com/serverless/serverless/discussions/10670#discussioncomment-2141418

For the second one, see the note under progress messages in their docs:

Users should know which plugin is working from the progress message:
    Bad: "Compiling"
    Bad: "[Webpack] Compiling" (avoid prefixes)
    Good: "Compiling with webpack"

If I'm being too picky, I'm more than happy to submit a PR myself.

miguel-a-calles-mba commented 2 years ago

Thanks for the feedback, @coyoteecd. I pushed an update for your review.

coyoteecd commented 2 years ago

Looks good now, thank you for the update. Could you publish the change to npm?

miguel-a-calles-mba commented 2 years ago

@coyoteecd, will do soon. Were you able to test it by installing the PR? Let me know if that worked and I will publish to npm. Thanks!

coyoteecd commented 2 years ago

@miguel-a-calles-mba yes, I tested it (with latest sls), both with and without --verbose argument.

miguel-a-calles-mba commented 2 years ago

@coyoteecd, the package is now in npm. Thanks for your help!