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

Use effective profile instead of an entirely new default profile when loading profiles #1193

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Dec 3, 2024

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.

@tgreenx tgreenx added the V-Patch Versioning: The change gives an update of patch in version. label Dec 3, 2024
@tgreenx tgreenx added this to the v2024.2 milestone Dec 3, 2024
@tgreenx tgreenx mentioned this pull request Dec 3, 2024
@tgreenx tgreenx linked an issue Dec 3, 2024 that may be closed by this pull request
matsduf
matsduf previously requested changes Dec 3, 2024
Copy link
Contributor

@matsduf matsduf left a 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.

@tgreenx
Copy link
Contributor Author

tgreenx commented Dec 4, 2024

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.

@tgreenx tgreenx modified the milestones: v2024.2, v2025.1 Dec 4, 2024
@matsduf matsduf dismissed their stale review December 4, 2024 09:21

Fine for v2025.1 release. Needs reviewing and testing first.

@mattias-p
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit test fails in develop branch
3 participants