Adding the Service Logs Streaming feature

[fabiobrz]

Fix #474

CC @mchoma, @simkam, @tommaso-borgato

Please make sure your PR meets the following requirements:

• Pull Request contains a description of the changes
• Pull Request does not include fixes for multiple issues/topics
• Code is formatted, imports ordered, code compiles and tests are passing
• Code is self-descriptive and/or documented

[mchoma]

I am looking forward to use this feature :)

[mchoma]

I could not resist to use it immediatelly :) and it works!!! Just colouring does not work in my case, but could be just me.

[fabiobrz]

I could not resist to use it immediatelly :) and it works!!! Just colouring does not work in my case, but could be just me.

Thanks for your feedback @mchoma ! I'd like to check about the coloring thing, how did you enable the feature? e.g.: did you pass xtf.sls.enabled=true? This is for me to understand whether logs are sent to STDOUT or to a file.

Actually this wouldn't change a thing because I've been using cat to see a generated log file and I could see colors there as well, but I'd focus on your use case to double check.

[mnovak1]

@fabiobrz I like this idea. This is something we miss and will be useful for everyone. Thanks for raising PR. I'll review it as soon as possible :-)

[simkam]

Could you please review the visibility of classes? It looks like all are public but are only used by extension within the package, perhaps they can be changed to package-private? Let's expose as minimum as possible.

[fabiobrz]

Could you please review the visibility of classes? It looks like all are public but are only used by extension within the package, perhaps they can be changed to package-private? Let's expose as minimum as possible.

Will do, thanks @simkam

[mnovak1]

@fabiobrz For me there was no issue to enable the feature and have colored text on standard output. I wanted to try how it will behave when forwarded to a file like -Dxtf.sls.config="target=.*Test;output=/home/mnovak/sls-logs.txt" but no /home/mnovak/sls-logs.txt file was created. It's still logged to standard output. Is there anything else needed to configure?

[fabiobrz]

@fabiobrz For me there was no issue to enable the feature and have colored text on standard output. I wanted to try how it will behave when forwarded to a file like -Dxtf.sls.config="target=.*Test;output=/home/mnovak/sls-logs.txt" but no /home/mnovak/sls-logs.txt file was created. It's still logged to standard output. Is there anything else needed to configure?

Hi @mnovak1 - Actually output should point to a directory which is meant to be the base path in which files for each executed test class will be created.
There are (at least) a couple of issues, though:

1. the implementation should create such path if it does not exist (?)
2. it should be better documented that the expected path is about a directory rather than a file.

Could you please take such information into account and try again? I had my files generated and would appreciate a confirmation from you.
I'll fix what above as soon as I push something to implement the changes in the feature activation logic that have been previously requested.

[simkam]

wrt feature activation, I think that only removing it from services and documenting it should be enough. Just to keep it consistent with other extensions.

[fabiobrz]

wrt feature activation, I think that only removing it from services and documenting it should be enough. Just to keep it consistent with other extensions.

Thanks @simkam - so basically according to your suggestion, the users should:

• register the extension, e.g. adding the FQN to services locally
• use the system property or annotation to enable the feature when running tests

Correct?

[simkam]

Yes, exactly

[mnovak1]

Hi @mnovak1 - Actually output should point to a directory which is meant to be the base path in which files for each executed test class will be created. There are (at least) a couple of issues, though:

1. the implementation should create such path if it does not exist (?)
2. it should be better documented that the expected path is about a directory rather than a file.

Could you please take such information into account and try again? I had my files generated and would appreciate a confirmation from you. I'll fix what above as soon as I push something to implement the changes in the feature activation logic that have been previously requested.

Thanks, now it works. +1 for both points.

[fabiobrz]

Could you please review the visibility of classes? It looks like all are public but are only used by extension within the package, perhaps they can be changed to package-private? Let's expose as minimum as possible.

Will do, thanks @simkam

About this, @simkam - actually the extension lives in a different module and package as such, hence by a quick check, only PodLogsWatcher could be package-private. Am I missing something about this specific request?

[simkam]

About this, @simkam - actually the extension lives in a different module and package as such, hence by a quick check, only PodLogsWatcher could be package-private. Am I missing something about this specific request?

No, sorry, I didn't notice extension is in different package.

[fabiobrz]

Hi @simkam - I pushed some changes in order to address your comments. Feel free to let me know whether you've got any additional feedback.

[simkam]

LGTM

[mnovak1]

@fabiobrz There should be no regressions. Merging. Thanks for the PR.