refactor(CLI): Replace warnings with modern counterparts #10080
Conversation
lib/classes/PluginManager.js
Outdated
| @@ -133,7 +133,10 @@ class PluginManager { | |||
|
|
|||
| // don't load plugins twice | |||
| if (this.plugins.some((plugin) => plugin instanceof Plugin)) { | |||
| this.serverless.cli.log(`WARNING: duplicate plugin ${Plugin.name} was not loaded\n`); | |||
| legacy.log(`WARNING: duplicate plugin ${Plugin.name} was not loaded\n`); | |||
| log.warning( | |||
There was a problem hiding this comment.
This in my opinion should also stay as a warning - we can succesfully continue with that configuration so it makes no sense to convert it to error, but on the other hand we should let people know that they have duplicated plugins definitions. I've adjusted the copy a little bit as well
There was a problem hiding this comment.
(in v3) why not making it an error? I don't see a reason to continue the execution here?
Is there a scenario where this bad configuration is "normal"?
There was a problem hiding this comment.
Hm, this is just a slight configuration issue where someone by mistake specified the plugin more than once - I don't think it should be a hard error as we can move forward with execution and just warn the user about that fact. I guess we could error out but it seemed a bit too strict for me
There was a problem hiding this comment.
Crashing feels justified if there's a significant risk of following with something not intended by the user, or if simply there's no way to follow with the proposed configuration.
This configuration error feels like an unintended omission that's safe to continue with.
There was a problem hiding this comment.
Warnings are confusing by nature: something's wrong, but not really. The execution is continuing on an unclear state (from the user POV, not necessarily the technical POV).
Errors are simple: either it's good, either it isn't.
Accomodating errors into warnings makes sense if we ease the life of a major slice of the userbase (e.g. when introducing config validation: the existing errors are just too much, it's too much of an inconvenience to catch up for the whole userbase).
But we should avoid warnings as much as possible:
- the are confusing
- they add noise to the output
Here it's a clear case of "something is wrong". Sure we can continue, but that doesn't mean we should. The impacted userbase is, I'd expect, also very limited.
So if we can avoid making it a warning, is it better:
- a verbose log?
- a deprecation?
- or an error?
Sounds to me like error is the best fit (but could be verbose log too?)
There was a problem hiding this comment.
@mnapoli and why exactly it's harmful to have it covered with deprecation warning instead of regular warning?
There was a problem hiding this comment.
@mnapoli we'd love to merge it. Still it'll be great first to clarify the logical reason behind introducing breaking change without deprecation.
I don't understand the reason, can you explain what logic drives your decision in this matter?
There was a problem hiding this comment.
You've asked me to confirm (which I did), and then to double confirm (which I did again) ;) and I explained the reasons just above:
from the information we have (should be extremely rare, users should already see an explicit warning on every command, and we have no knowledge of a valid use case for this) I think it's reasonable
I don't think we should block this PR because we are not introducing any BC break in this PR.
If you want we can discuss it at the meeting in a couple of hours?
Sounds good?
There was a problem hiding this comment.
from the information we have (should be extremely rare, users should already see an explicit warning on every command, and we have no knowledge of a valid use case for this) I think it's reasonable
It translates to me to:
It's rare, so it's reasonable to introduce breaking change without deprecation message.
I don't understand why fact that it's rare invalidates the need for deprecation messages? What's the rationale behind it?
In other words:
Why do you think it's wrong to configure a deprecation message for extremeley rare case, yet you feel it's important to introduce breaking change for same case?
That's what I'm failing to understand.
This approach technically breaks the contract on our side. So far, no matter if rare or extremely rare breaking changes where accompanied with deprecation warnings and not just warnings. I fail to see a logical reason why this case should be different
Anyway feel free to explain in a meeting
There was a problem hiding this comment.
Additionally, we don't know how rare this case in reality is. We assume it's very rare.
And configuring deprecation message will give us both:
- Answer on how rare it is
- Ensure that in those rare cases, users are properly guided before upgrading to new release.
Codecov Report
@@ Coverage Diff @@
## master #10080 +/- ##
==========================================
- Coverage 85.38% 85.24% -0.15%
==========================================
Files 333 334 +1
Lines 13570 13634 +64
==========================================
+ Hits 11587 11622 +35
- Misses 1983 2012 +29
Continue to review full report at Codecov.
|
pgrzesik
force-pushed
the
warnings-modern-logs
branch
3 times, most recently
from
dc231ca
to
1da0781
Compare
pgrzesik
force-pushed
the
warnings-modern-logs
branch
from
1da0781
to
50bc4c5
Compare
pgrzesik
force-pushed
the
warnings-modern-logs
branch
2 times, most recently
from
24d25b2
to
c4207bd
Compare
| @@ -143,7 +145,8 @@ module.exports = { | |||
| ` configuration in your function "${functionName}".`, | |||
| ' Serverless will remove this configuration automatically before deployment.', | |||
| ].join(''); | |||
| this.serverless.cli.log(warningMessage); | |||
| legacy.log(`Warning! ${warningMessage}`); | |||
| log.warning(warningMessage); | |||
There was a problem hiding this comment.
This warning message will end to have Warning! prefix. Also it'll be good to put message for modern logs directly into log.warning invocation
There was a problem hiding this comment.
great catch, fixed
pgrzesik
force-pushed
the
warnings-modern-logs
branch
from
c4207bd
to
4fe5a86
Compare
pgrzesik
force-pushed
the
warnings-modern-logs
branch
3 times, most recently
from
6410559
to
4cba05e
Compare
There was a problem hiding this comment.
@pgrzesik looks good. Still please see my comments. I don't think that deprecation warnings should be accompanied with regular warnings (just deprecation warning is good enough I believe, and in such case we should also remove legacy log completely)
lib/classes/PluginManager.js
Outdated
| log.warning( | ||
| `Duplicate plugin definition found in your configuration. Plugin "${Plugin.name}" will not be loaded more than once.` | ||
| ); |
There was a problem hiding this comment.
This warning I believe should be gone now (?)
| @@ -136,6 +136,7 @@ describe('#generateCoreTemplate()', () => { | |||
|
|
|||
| return runServerless({ | |||
| config: { | |||
| disabledDeprecations: 'S3_TRANSFER_ACCELERATION_ON_EXISTING_BUCKET', | |||
There was a problem hiding this comment.
I think it'll be better to change this test, to one that confirms on deprecation in such scenario.
So Ideally we should leave it to throw and confirm on error code, e.g. we have similar test here:
serverless/test/unit/lib/Serverless.test.js
Lines 245 to 256 in f4163d2
pgrzesik
force-pushed
the
warnings-modern-logs
branch
from
4cba05e
to
5f40e36
Compare
I've started a discussion about What do you think @medikoo ? |
Good point, I missed that it follows |
Addresses: #9860