-
Notifications
You must be signed in to change notification settings - Fork 24
Use effective profile instead of an entirely new default profile when loading profiles #1193
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
base: develop
Are you sure you want to change the base?
Conversation
… loading profiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are late in the testing cycle for the v2024.2 release. This PR affects how things are tested whereas #1192 only affects how the unit tests are run. Wouldn't it be safer to merge #1192 now and then, after the release merge this unless we find any issues with it? This would mean that we do not have to review and test this before the release.
If/when this is merged, t/test01.t could be cleaned up and the changes from #1192 could be reverted. The t file could win on some restructuring anyway.
Yes I agree, fine with me. |
Fine for v2025.1 release. Needs reviewing and testing first.
|
While I haven't yet looked into the problem or how this change would solve it, the change is giving me bad vibes. I need to take a closer look, but I'll get back to it later since it's been pushed to the next release. |
|
In the config documentation we define the "available profiles" to be relative to the default profile. Before this change load_profiles() returned the "available profiles" regardless of the current settings in the effective profiles. After this change, if you want the "available profile", the caller is responsible for resetting the effective profile to the default profile before calling load_profiles(). I we do want this behavior I think it should be more clearly documented. If possible I'd like to keep the current behavior of load_profiles(). |
|
If this PR changes the behavior as specified in https://doc.zonemaster.net/latest/configuration/backend.html#public-profiles-and-private-profiles-sections or https://metacpan.org/pod/Zonemaster::Engine::Profile#PROFILE-PROPERTIES than those should be updated first. |
|
@tgreenx, is this to be updated or closed? |
It should be reviewed, especially by @mattias-p. Maybe the change is not good, but #1192 was supposed to be a temporary fix only (see comments in that PR, especially #1192 (review)). |
Purpose
This PR proposes to use effective profile instead of an entirely new default profile when loading profiles in Backend. This means that missing values are filled from the effective profile instead of the default profile, in case any were changed. Note that the effective profile is initially instantiated from the default profile.
Context
See #1192 (review).
Fixes #1191
Replaces #1192
How to test this PR
Unit tests are updated and should pass.