search

gocsaf / csaf

Tools to download or provide CSAF (Common Security Advisory Framework) documents.
https://csaf.io
40 stars 23 forks source link

Added support for structured logging in `csaf_aggregator` #530

Closed oxisto closed 7 months ago

oxisto commented 7 months ago

This PR adds structured logging for the aggregator service. Currently, only the text handler is used, but I can extend this to use the JSON handler as well. In this case, probably some code that is shared between the aggregator and the downloader would need to be moved to a common package.

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

Fixes #462

s-l-teichmann commented 7 months ago

Thank for your contribution. I will review the PR as soon as I've git some time.

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

We clearly can do this as we already where there. The 'x/exp' stuff was only introduced in PR #514 to help backwards compatibility.

oxisto commented 7 months ago

Thank for your contribution. I will review the PR as soon as I've git some time.

Looking forward to the feedback!

I was also wondering, whether this repo is moving to Go 1.21 at the future, since slog was introduced in to the standard lib in 1.21. So currently, this still relies on the x/exp package.

We clearly can do this as we already where there. The 'x/exp' stuff was only introduced in PR #514 to help backwards compatibility.

Ah I see. Well it should be more or less a case of just replacing the exp package with the slog package, once you decide to revisit Go 1.21.

tschmidtb51 commented 7 months ago

Just a reminder: When we update the Go version, this also needs to be documented in https://github.com/csaf-poc/csaf_distribution/blob/main/README.md#build-from-sources and https://github.com/csaf-poc/csaf_distribution/blob/main/docs/Development.md#supported-go-versions as well.

bernhardreiter commented 7 months ago

@oxisto are you okay with the new license Apache-2.0 for your contribution #535 ?

Can you also do the Go Version update has part of this PR?

oxisto commented 7 months ago

@oxisto are you okay with the new license Apache-2.0 for your contribution #535 ?

Yes, that is ok for me.

Can you also do the Go Version update has part of this PR?

Sure, I can do that and already use slog instead of the x/exp one.

bernhardreiter commented 7 months ago

@oxisto a side question about Github: Is it possible for your to run the "Integration Test" workflow (https://github.com/oxisto/csaf_distribution/blob/main/.github/workflows/itest.yml ) because obviously I cannot do this on your clone and as it is not part of our automatic test I have not figured out how to run it on a pull request manually).

bernhardreiter commented 7 months ago

@oxisto in one commit message you are asking a few questions (which ideally shouldn't be in a commit message):

but I can extend this to use the JSON handler as well.

It should be similar to how the downloader handels it.

In this case, probably some code that is shared between the aggregator and the downloader would need to be moved to a common package.

Yes common code should go into a common package, we already have util.

Both can be done in different PRs.

oxisto commented 7 months ago

@oxisto in one commit message you are asking a few questions (which ideally shouldn't be in a commit message):

but I can extend this to use the JSON handler as well.

It should be similar to how the downloader handels it.

In this case, probably some code that is shared between the aggregator and the downloader would need to be moved to a common package.

Yes common code should go into a common package, we already have util.

Both can be done in different PRs.

Sure, once this is merged I can have a look at what can be extracted into a common package, so that other tools can potentially use this as well and that behaviour is consistent across cmd.

Apart from that I have changed the symbol to sym link.

oxisto commented 7 months ago

@oxisto a side question about Github: Is it possible for your to run the "Integration Test" workflow (https://github.com/oxisto/csaf_distribution/blob/main/.github/workflows/itest.yml ) because obviously I cannot do this on your clone and as it is not part of our automatic test I have not figured out how to run it on a pull request manually).

https://github.com/oxisto/csaf_distribution/actions/runs/8821657205/job/24218013269