serverless / compose

Orchestrate Serverless Framework in monorepos
https://serverless.com/framework/docs/guides/compose
MIT License
111 stars 15 forks source link

Improvements to handling `outputs` and `info` #60

Closed pgrzesik closed 2 years ago

pgrzesik commented 2 years ago

This PR adds the following changes:

As for the info command, I'd like to ask your opinion on how we should represent it:

Please see at some of the attached outputs for different scenarios:

Success with progresses:

52647C2F-E330-4F45-AFBC-CD96E52DB9CB

Partial failure with progresses:

CB950561-13F6-4BBE-8FF4-1EBB312DC094_1_105_c

Success without progresses:

6067BB56-35B1-49F3-8440-AABA95C6C543_1_105_c

The scenario without progress with errors is messy - there's no good place to highlight the thrown error. I think the option with progresses is more clear in case of error, but displaying the progresses at the end feels like it's not really needed - what do you think @mnapoli ?

Addresses: #55

mnapoli commented 2 years ago
  • Should we include the progress spinners when executing the command?
  • If yes, should we display them afterward?

I don't think we need spinners at all (if we ignore the error scenario).

  • How should we handle errors for specific sub-commands? E.g. when one fails?

I would go the easy route and crash the whole command (i.e. we don't even get a partial view of the info) -> is there a scenario where 1 service could crash, but it's "normal" and we don't want to block showing info for the other services?

I don't see one (maybe I'm missing it), that's why I'd say if there's an error, it's probably the most important thing and it should be fixed before anything else will work.

Separately from that, I don't think we should show "verbose" info by default (that's what i see in the screenshots at least). I.e. slsc info should show sls info, and slsc info --verbose should maybe show sls info --verbose? (+ verbose logs above)

pgrzesik commented 2 years ago

I would go the easy route and crash the whole command (i.e. we don't even get a partial view of the info) -> is there a scenario where 1 service could crash, but it's "normal" and we don't want to block showing info for the other services?

This is unfortunately not compatible with the approach where the component method is responsible for writing out the output - let's assume situation like this:

  1. We have 3 services and run info
  2. 2 services finish and print out the outputs
  3. 3rd service crashes

In that scenario, we already have partial output emitted so we cannot crash the whole command. Do you think that in such case, we should just crash the command?

Alternatively, we can execute everything and report errors for failed commands, but highlight it a bit better, e.g.

8726DBAA-AD1A-4060-A269-16E4334EADBE_1_105_c

We clearly state that execution for service X failed and print out the error (of course wording to be adjusted)

As yet another alternative, we can highlight that the error happened by using namespaced logging like this:

4C207BDD-BFB4-408A-9270-A8E8B01500EF_1_105_c

What do you think @mnapoli ?

Separately from that, I don't think we should show "verbose" info by default (that's what i see in the screenshots at least). I.e. slsc info should show sls info, and slsc info --verbose should maybe show sls info --verbose? (+ verbose logs above)

Yep, this should be supported as you mention 👍

pgrzesik commented 2 years ago

One note on the verbose - unfortunatelly, at the moment it's currently unusable and I think we will in general have to separate debug logs from verbose logs - for example, currently on verbose we print out all the outputs from commands executed via exec on components and in this particular case it just duplicates the output, because the result of the command is printing the output of the executed command.

So we end up with 48512870-91A9-4208-961E-0437B4609E98_1_105_c something like this:

mnapoli commented 2 years ago

Regarding errors:

Good points. I prefer the first version, where the error is at the root, with an "error message prefix".

Ideally it should be like this though (to match the Framework error formatting):

Error:
Execution of "info" failed for service "resources": Configuration error at root ...

Regarding --verbose:

The original goal of having subprocess output in verbose logs was for deploy (to have the full details of the deployment).

But I agree that when we re-print the output (e.g. for logs, info, etc.), this is messy. Maybe we should have an option on the "exec()" internal method so that we can control it, e.g. deploy logs the subprocess output to verbose, but other methods can choose not to do that?