shivam-51: Add log file paths to dapr list 1228

[shivam-51]

Description

Add paths of app and dapr logs to dapr list command.

Issue reference

Please reference the issue this PR will close: #1228

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[mukundansundar]

@shivam-51
Welcome to the community. Thanks for the contribution!
Can you please add E2E for this change?
Please see #1200 for reference.

[codecov]

Codecov Report

Merging #1296 (1335858) into master (fa2c99d) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1296      +/-   ##
==========================================
- Coverage   26.87%   26.83%   -0.05%     
==========================================
  Files          39       39              
  Lines        3873     3879       +6     
==========================================
  Hits         1041     1041              
- Misses       2758     2764       +6     
  Partials       74       74              

Impacted Files	Coverage Δ
pkg/standalone/list.go	0.00% <0.00%> (ø)

[pravinpushkar]

Multi app run template introduces log destination fields for daprd and app. We have to take into account if the log destination is set as console then what values it captures.
@shivam-51

[shivam-51]

Multi app run template introduces log destination fields for daprd and app. We have to take into account if the log destination is set as console then what values it captures. @shivam-51

@pravinpushkar Can you share some documentation for this property? I am not able to understand it's significance.
I checked https://docs.dapr.io/developing-applications/local-development/multi-app-dapr-run/multi-app-template/

[mukundansundar]

m not able to understand it's sign

PLease see this PR dapr/docs#3461

[shivam-51]

Multi app run template introduces log destination fields for daprd and app. We have to take into account if the log destination is set as console then what values it captures. @shivam-51

• For appLogDestination: console show appLogPath: console
• For appLogDestination: file show appLogPath: <file path>
• For appLogDestination: fileAndConsole show appLogPath: <file path> giving file more priority than console

@pravinpushkar @mukundansundar What do you think?

[philliphoff]

@shivam-51 @pravinpushkar @mukundansundar My vote would be that, if the log is written to a file, then add/populate the appLogDestination metadata property. If the log is not (e.g. being solely written to the console), then simply omit that metadata property (or set to null or the empty string ""). I would rather not have "special strings" that tooling needs to distinguish from otherwise valid paths.

[pravinpushkar]

@shivam-51 @pravinpushkar @mukundansundar My vote would be that, if the log is written to a file, then add/populate the appLogDestination metadata property. If the log is not (e.g. being solely written to the console), then simply omit that metadata property (or set to null or the empty string ""). I would rather not have "special strings" that tooling needs to distinguish from otherwise valid paths.

+1 to update log paths only when written to only files. In other cases the logs will anyway be available to the console and tolling can use that.

@philliphoff However one question - if there are multiple apps writing logs to console then how will the tooling distinguish which log written by which app ?

[philliphoff]

However one question - if there are multiple apps writing logs to console then how will the tooling distinguish which log written by which app ?

@pravinpushkar If the application output is only going to the console, then it'd be unavailable to tooling anyway (unless it was the one to have started the Dapr process and captured the stdout itself). If no path metadata is present, then tooling would simply assume it's being captured someplace else and not offer the user the option to view the logs.

[shivam-51]

Can you please add E2E for this change? Please see #1200 for reference.

@mukundansundar I have identified the tests need to go into list_test.go but I am not able to test locally. Most of the tests fail somehow.

[mukundansundar]

Can you please add E2E for this change? Please see #1200 for reference.

@mukundansundar I have identified the tests need to go into list_test.go but I am not able to test locally. Most of the tests fail somehow.

Could you explain what errors that you are seeing?

[shivam-51]

Could you explain what errors that you are seeing?

That's okay. I ran the tests on my fork using github actions.

[mukundansundar]

@shivam-51 Almost there, thanks for adding the negative scenarios. Can you add the positive test case as part of the run file scenarios where the list is actually populated with the file path? Inside this function here https://github.com/dapr/cli/blob/master/tests/e2e/standalone/stop_with_run_template_test.go#L121

[shivam-51]

@mukundansundar can you have a look please?