Minor improvements

[Marcono1234]

Contains some of the commits which were originally part of #207.

[sjoerdtalsma]

Aside from the small logger issue this looks very good to me, thanks!

[sjoerdtalsma]

Okay, let's just make all loggers static.

…

Op 14 mrt. 2021, om 23:37 heeft Marcono1234 ***@***.***> het volgende geschreven: @Marcono1234 commented on this pull request. In context-propagation-java5/src/main/java/nl/talsmasoftware/context/executors/ContextAwareExecutorService.java <#217 (comment)>: > @@ -39,7 +39,7 @@ * @author Sjoerd Talsma */ public class ContextAwareExecutorService extends CallMappingExecutorService { - private final Logger logger = Logger.getLogger(getClass().getName()); + private static final Logger logger = Logger.getLogger(ContextAwareExecutorService.class.getName()); The main point of my comment was rather why I prefer a static logger. There are probably now these options (1, 1.i, 2, 2.i, 3), which one do you prefer? Keep the logger code as is (i.e. ContextAwareExecutorService.logger remains an instance field) Additionally make ServletRequestContextFilter.logger an instance field as well and use getClass().getName() Keep ContextAwareExecutorService.logger as instance field but use ContextAwareExecutorService.class (addresses some of the concerns listed in my comment above) Additionally make ServletRequestContextFilter.logger an instance field but use ServletRequestContextFilter.class Make ContextAwareExecutorService.logger static — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#217 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWLNLHPKPHQR3EOE7DUAMLTDU3BVANCNFSM4YV6ZF2A>.

[sjoerdtalsma]

Could you do me a favour and merge develop into your branch?
I switched to Github Actions and I'm curious how the build runs on a merge request (from a forked repository).

[Marcono1234]

Sorry for the delay, I have merged develop into this branch now and made the logger in AbstractThreadLocalContext static as well.

I think you need to include pull_request as event which triggers the workflow. (Probably also does not matter whether a pull request performs a merge from develop or not.)

You can probably also slightly simplify the workflow by removing the following (though I am not completely sure):

context-propagation/.github/workflows/ci-build.yml

Lines 6 to 7 in a12ec68

	branches:
	- '**'

[sjoerdtalsma]

I think you need to include pull_request as event which triggers the workflow. (Probably also does not matter whether a pull request performs a merge from develop or not.)

Thanks, I wasn't 100% sure about that, but since your workflow didn't run I guess it's necessary.

You can probably also slightly simplify the workflow by removing the following (though I am not completely sure):

context-propagation/.github/workflows/ci-build.yml

Lines 6 to 7 in a12ec68

	branches:
	- '**'

I tried, but it refused to run the workflow on develop. This probably has to do with the exclusion of tags somehow. I just re-enabled it again for now.

[Marcono1234]

Force pushed (only changing commit message and reverting it back) to trigger GitHub workflow.

[sjoerdtalsma]

I need to make the tasks that depend on secrets skippable for pull requests.
I'm glad no secrets were shared with a 'foreign' fork for reasons you can probably understand ;-)

Sorry for all the trial and error in this PR!

[sjoerdtalsma]

Updated the build script, should run from forked repo now.

[Marcono1234]

Sorry for all the trial and error in this PR!

No worries; looks like it worked now.

[sjoerdtalsma]

Merged to develop. Thanks again for your contribution @Marcono1234