Skip to content

Conversation

branic
Copy link
Collaborator

@branic branic commented Sep 16, 2025

What does this PR do?

Removes setting the default for the aap_configuration_collect_logs in every role and sets the default in the only location the variable is used.

How should this be tested?

Manually

Is there a relevant Issue open for this?

No

Other Relevant info, PRs, etc

@branic branic requested a review from a team as a code owner September 16, 2025 01:11
@branic
Copy link
Collaborator Author

branic commented Sep 16, 2025

There are a couple of other minor inconsistencies that I also fixed.

@djdanielsson
Copy link
Collaborator

Why move away from the defaults file? Just not sure I understand the value of this change

@branic
Copy link
Collaborator Author

branic commented Sep 16, 2025

To simplify. The same value was being set as the default in every role which is lots of redundant code. This particular variable doesn't have role specific variants so there isn't a need to set it in each role. By specifying the default in the only place it is used ensures that there is always a default (even when new roles are added) and eliminates all the redundant code.

Had I realized I was creating so much duplication when I was working on the PR that introduced the async logging I would have done this then, but there were so many variable changes I didn't see the forest through the trees until I started implementing the async logging change in the infra.controller_configuration collection.

Copy link
Collaborator

@djdanielsson djdanielsson left a comment

Choose a reason for hiding this comment

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

LGTM

@djdanielsson djdanielsson merged commit 4aea715 into redhat-cop:devel Sep 16, 2025
4 checks passed
@branic branic deleted the update_async_logging branch September 16, 2025 14:28
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.

2 participants