feat(CLI): Modern logs for `package` command #9956

pgrzesik · 2021-09-14T15:15:24Z

Addresses: #9860

pgrzesik · 2021-09-14T15:16:15Z

lib/plugins/aws/package/index.js

@@ -102,6 +117,11 @@ class AwsPackage {
        await this.saveServiceState();
        return this.serverless.pluginManager.spawn('aws:common:moveArtifactsToPackage');
      },
+
+      'after:aws:package:finalize:saveServiceState': () => {


I'm a bit on the fence here - I was thinking about adding a new lifecycle hook that runs last instead of attaching with after - what is your opinion @medikoo ?

Yes, maybe let's add finalize lifecycle hook (sort of counterpart of initialize), it'll be run only in case of command success (otherwise we have error)

codecov · 2021-09-14T15:21:28Z

Codecov Report

Merging #9956 (eaa1030) into master (aaabb50) will decrease coverage by 0.04%.
The diff coverage is 70.58%.

❗ Current head eaa1030 differs from pull request most recent head f407287. Consider uploading reports for the commit f407287 to get more accurate results

@@            Coverage Diff             @@
##           master    #9956      +/-   ##
==========================================
- Coverage   86.20%   86.15%   -0.05%     
==========================================
  Files         329      330       +1     
  Lines       13032    13047      +15     
==========================================
+ Hits        11234    11241       +7     
- Misses       1798     1806       +8

Impacted Files	Coverage Δ
lib/classes/PluginManager.js	`93.69% <54.54%> (-1.01%)`	⬇️
lib/plugins/aws/package/index.js	`100.00% <100.00%> (ø)`
.../templates/aws-nodejs-typescript/webpack.config.js	`0.00% <0.00%> (ø)`
lib/utils/logDeprecation.js	`95.58% <0.00%> (+1.06%)`	⬆️

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 aaabb50...f407287. Read the comment docs.

medikoo · 2021-09-15T08:06:35Z

lib/plugins/aws/package/index.js

+        const isPackageCommand =
+          this.serverless.processedInput.commands.length === 1 &&
+          this.serverless.processedInput.commands[0] === 'package';


Shorter form could be: this.serverless.processedInput.commands.join(" ") === "package"

medikoo · 2021-09-15T08:08:05Z

lib/plugins/aws/package/index.js

@@ -102,6 +117,11 @@ class AwsPackage {
        await this.saveServiceState();
        return this.serverless.pluginManager.spawn('aws:common:moveArtifactsToPackage');
      },
+
+      'after:aws:package:finalize:saveServiceState': () => {


Yes, maybe let's add finalize lifecycle hook (sort of counterpart of initialize), it'll be run only in case of command success (otherwise we have error)

pgrzesik · 2021-09-15T09:25:20Z

lib/plugins/aws/package/index.js

@@ -102,6 +114,11 @@ class AwsPackage {
        await this.saveServiceState();
        return this.serverless.pluginManager.spawn('aws:common:moveArtifactsToPackage');
      },
+
+      'success': () => {


I've added a success hook as finalize is already very widely used and I didn't want to create confusion. I'm not fully sold on the name though so I'm up for suggestions

Yes finalize is used as a token for many lifecycle event groups, but technically it's similar to initialize (it's used less, as doesn't have as many use cases, but it's used)

Having I think it'll be consistent to follow global initialize with global finalize as it follows terminology that's used with hooks. success is something new and its semantics doesn't necessarily indicate it's about something to be invoked at the end.

changed to finalize 👍

pgrzesik · 2021-09-15T09:26:25Z

lib/classes/PluginManager.js

@@ -638,6 +638,7 @@ class PluginManager {

    try {
      await this.invoke(commandsArray);
+      for (const { hook } of this.getHooks(['success'])) await hook();


I'm not sure about placement of this - I wanted to makes sure that it's only ran if there are not errors, but also ensure that we run error hooks if success hook fails. Do you think we should handle it this way or not run error hook on success hook failure?

Good point.

I think it should be consistent with how we handle initialize hook, so error in it should not trigger error hook.

And as I inspect current logic, I find a bit surprising that we're processing backend request (showing notification) also when deploy crashes. Was that intended? Or was it accidentally added?

As look into it, I think proper logic should be as:

await runInitializeHooks(); deferredBackendNotificationRequest = handleTelemetry() try { await runNormalHooks(); } catch (error) { try { await runErrorHooks(); } catch { await deferredBackendNotificationRequest; throw error; } } try { await runFinalizeHooks(); } catch (error) { await deferredBackendNotificationRequest; throw error; } await processBackendNotificationRequest(await deferredBackendNotificationRequest);

That's good point that you bring up here with notifications - I think this might've been introduced by a mistake as I don't remember that we wanted to show notifications in case of errors - I'm also not sure how it behaved previously, do you remember?

As I check notifications were shown only on success, although logic may not look that clear:

serverless/lib/plugins/deploy.js

Lines 55 to 63 in bd4e792

} finally {

this.deferredBackendNotificationRequest = telemetryStoreLocally(

Object.assign(await generateTelemetryPayload(this.serverless), { command: 'deploy' })

);

}

},

'after:deploy:finalize': async () => {

if (!this.deferredBackendNotificationRequest) return null;

return this.deferredBackendNotificationRequest.then(processBackendNotificationRequest);

We were reporting telemetry before deploy logic (but unconditionally after packaging no matter if it succeeded or not), and the result was passed to backend notifications processor with last hook (and that one is not triggered if there's an error on the way)

Also logic we're discussing here, takes place only if there's a local fallback from older major release.

In other cases telemetry is handled in main script, and over there notification is only shown on success.
So we can treat it as bug here, that we're doing otherwise

Thanks for the clarification, that sounds like a bug to me then - should I adjust it here to only show notification in situations where the command did not throw an error?

Yes I would definitely do it. It can be two commits (one that fixes issue, and second that adds handling of final hook)

medikoo

I've changed a bit logic in pluginManager.run to something afaik bit cleaner (more comprehensible)

Additionally I've merged commits related to package logging into one commit

pgrzesik added enhancement cat/cli labels Sep 14, 2021

pgrzesik self-assigned this Sep 14, 2021

pgrzesik commented Sep 14, 2021

View reviewed changes

pgrzesik requested a review from medikoo September 14, 2021 15:24

medikoo suggested changes Sep 15, 2021

View reviewed changes

pgrzesik force-pushed the support-modern-logs-for-package branch from a6ff0b7 to 5eeb541 Compare September 15, 2021 08:46

pgrzesik commented Sep 15, 2021

View reviewed changes

pgrzesik requested a review from medikoo September 15, 2021 09:26

pgrzesik force-pushed the support-modern-logs-for-package branch 3 times, most recently from 3aeaded to eaa1030 Compare September 15, 2021 11:10

medikoo and others added 5 commits September 16, 2021 11:50

refactor: Improve var naming

903e13b

fix(Telemetry): Ensure to not show backed notification on error

f320563

style: Simplify

c9f7537

feat: Support finalize hook, triggered on command finalization

88b1cb7

feat(CLI): Modern logs for package command

f407287

medikoo force-pushed the support-modern-logs-for-package branch from eaa1030 to f407287 Compare September 16, 2021 09:58

medikoo approved these changes Sep 16, 2021

View reviewed changes

medikoo merged commit fa2507d into master Sep 16, 2021

medikoo deleted the support-modern-logs-for-package branch September 16, 2021 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CLI): Modern logs for `package` command #9956

feat(CLI): Modern logs for `package` command #9956

pgrzesik commented Sep 14, 2021

pgrzesik Sep 14, 2021

medikoo Sep 15, 2021

codecov bot commented Sep 14, 2021 •

edited

medikoo Sep 15, 2021

medikoo Sep 15, 2021

pgrzesik Sep 15, 2021

medikoo Sep 15, 2021 •

edited

pgrzesik Sep 15, 2021

pgrzesik Sep 15, 2021

medikoo Sep 15, 2021

pgrzesik Sep 15, 2021

medikoo Sep 15, 2021 •

edited

medikoo Sep 15, 2021

pgrzesik Sep 15, 2021

medikoo Sep 15, 2021

medikoo left a comment

	} finally {
	this.deferredBackendNotificationRequest = telemetryStoreLocally(
	Object.assign(await generateTelemetryPayload(this.serverless), { command: 'deploy' })
	);
	}
	},
	'after:deploy:finalize': async () => {
	if (!this.deferredBackendNotificationRequest) return null;
	return this.deferredBackendNotificationRequest.then(processBackendNotificationRequest);

feat(CLI): Modern logs for package command #9956

feat(CLI): Modern logs for package command #9956

Conversation

pgrzesik commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 14, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo Sep 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo Sep 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

feat(CLI): Modern logs for `package` command #9956

feat(CLI): Modern logs for `package` command #9956

codecov bot commented Sep 14, 2021 •

edited

medikoo Sep 15, 2021 •

edited

medikoo Sep 15, 2021 •

edited