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
Remove Enterprise and Platform from log info #6501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems a bit ambiguous, but if this was product wants,
@@ -330,7 +330,7 @@ class CLI { | |||
|
|||
getVersionNumber() { | |||
this.consoleLog( | |||
`${version} (Enterprise Plugin: ${enterpriseVersion}, Platform SDK: ${sdkVersion})` | |||
`Framework Core: ${version}\nPlugin: ${enterpriseVersion}\nSDK: ${sdkVersion}\n` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. seems a bit to ambiguous now thb... what about Dashboard Plugin
and Dashboard SDK
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the preference is to leave it somewhat ambiguous, in order to keep things more future-proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here 🤔
What if we use the npm package names?
serverless: 1.49.0
@serverless/enterprise-plugin: 1.2.3
@serverless/platform-sdk: 1.2.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also agree with what @dschep said.
Anyway, we can merge it since it's not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixes the tests. Travis should be happy and we should be able to merge this ASAP.
What did you implement:
Removed
Enterprise
andPlatform
from loglineHow did you implement it:
With the
delete
key.How can we verify it:
run
serverless -v
against this branch, or require this class in a node console and rungetVersionNumber()
Todos:
Note: Run
npm run test-ci
to run all validation checks on proposed changesValidate via
npm test
Validate via
npm run lint-updated
Note: Some reported issues can be automatically fixed by running
npm run lint:fix
Validate via
npm run prettier-check-updated
Note: All reported issues can be automatically fixed by running
npm run prettify-updated
Is this ready for review?: YES
Is it a breaking change?: NO