Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update audit-log presets #1446

Merged
merged 7 commits into from
Nov 26, 2024
Merged

update audit-log presets #1446

merged 7 commits into from
Nov 26, 2024

Conversation

sjvans
Copy link
Contributor

@sjvans sjvans commented Nov 15, 2024

re #1443

@@ -486,13 +490,12 @@ This provides an ultimate level of resiliency, plus additional benefits:

- **False log messages are avoided** — messages are forwarded to the audit log service on successfully committed requests; and skipped in case of rollbacks.

This transparently applies to all implementations, even [custom implementations](#custom-implementation). You can opt out of this default by configuring outbox: false in the configuration, for example, as we do in the default configuration for development:
This transparently applies to all implementations, even [custom implementations](#custom-implementation). You can opt out of this default by configuring outbox: false in the configuration, for example, as shown in the following snippet for profile `development`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using this format instead of the code block?
image

@renejeglinsky renejeglinsky merged commit 6657449 into main Nov 26, 2024
4 checks passed
@renejeglinsky renejeglinsky deleted the audit-log-presets branch November 26, 2024 08:58
@grenik
Copy link

grenik commented Nov 27, 2024

@renejeglinsky thanks for fixing it, but I think there are two open points left:

  1. I think Fix outdated audit logging config (#1442) #1443 (comment) from @sjvans regarding default outboxed was not reflected (it's not false anymore)

  2. I thought kind is the new level of abstraction from the internal implementation, which is there exactly to avoid such errors (plugin impl changed > config doesn't work anymore > documentation must be changed / code throws errors)
    See the audit logging plugin impl
    This is why I suggested to replace impl with kind in Fix outdated audit logging config (#1442) #1443
    Is my understanding wrong?

@renejeglinsky
Copy link
Contributor

Hi @grenik
for the latter: As this kind is the implicit default (as you can see from the source you linked or by executing cds env requires.audit-log), there's no need to list it here. That way, you'd not benefit from any changes done within the plugin. If you stick to the defaults, you benefit from all the changes there. At least that is my understanding. Maybe @sjvans can confirm or improve my answer.
Wrt your first point, thanks for catching this! I'll fix it.

@sjvans
Copy link
Contributor Author

sjvans commented Nov 28, 2024

i'm a little torn on this one. i don't really see the value in [development]: { kind: 'audit-log-to-console' } without also listing what this predefined kind contains. but if we also add the three kinds, then the snippet gets way too long. that's why i actually kind of like the current state, because it reflects the effective env. but it really is confusing for the readers...

@sjvans
Copy link
Contributor Author

sjvans commented Nov 28, 2024

maybe something like #1481?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants