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

Make queued auditing opt-in #882

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Dec 18, 2023

The previous PR #881 made the default behavior opt-out, while I think we actually want it to be opt-in to avoid breaking changes.

For some more context, I am updating composer packages and our test cases were failing that were concerned with auditing. After some digging, I realized I had to copy over the updates to config/audit.php and ensure that we had 'audit.queue.enabled' set to false.

With the change proposed in this PR, there's no need to copy over the configuration (which isn't always obvious), it will just work exactly as it did previously without updating the configuration.

@MortenDHansen
Copy link
Contributor

Agreed 👍

@MortenDHansen MortenDHansen merged commit 2d3510d into owen-it:master Dec 18, 2023
13 checks passed
@spaantje
Copy link

Please note that this change does not prevent the job from being queued. Because of the change in the main config it still queues the job when the config is published and not updated. You should also update the root config to false which was changed/added in #881

(config/audit.php > queue.enable)

@cangelis
Copy link

Please note that this change does not prevent the job from being queued. Because of the change in the main config it still queues the job when the config is published and not updated. You should also update the root config to false which was changed/added in #881

(config/audit.php > queue.enable)

100%. The default value should also be false in config/audit.php

There is a breaking change currently re queues and this PR doesn't really prevent the issue happening.

See: #890

@erikn69 erikn69 mentioned this pull request Dec 29, 2023
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.

4 participants