APP-1152 Add legacy fallback variable when finding persistence directory#13629
Merged
APP-1152 Add legacy fallback variable when finding persistence directory#13629
Conversation
Contributor
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Collaborator
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Pragmatic backward compatibility fix, but needs test coverage.
The core logic is solid and follows the "don't break userspace" principle. However, this adds new behavior without tests.
|
|
||
| # Legacy V0 fallback variable | ||
| if persistence_dir is None: | ||
| persistence_dir = os.getenv('FILE_STORE_PATH') |
Collaborator
There was a problem hiding this comment.
🟠 Important: This adds new behavior but there are no tests for get_default_persistence_dir().
You should add tests that verify:
FILE_STORE_PATHis used whenOH_PERSISTENCE_DIRis not setOH_PERSISTENCE_DIRtakes precedence overFILE_STORE_PATHwhen both are set- Default behavior when neither is set
Example test structure:
def test_persistence_dir_fallback_to_legacy(monkeypatch, tmp_path):
monkeypatch.delenv('OH_PERSISTENCE_DIR', raising=False)
monkeypatch.setenv('FILE_STORE_PATH', str(tmp_path))
result = get_default_persistence_dir()
assert result == tmp_path
def test_persistence_dir_precedence(monkeypatch, tmp_path):
new_dir = tmp_path / 'new'
legacy_dir = tmp_path / 'legacy'
monkeypatch.setenv('OH_PERSISTENCE_DIR', str(new_dir))
monkeypatch.setenv('FILE_STORE_PATH', str(legacy_dir))
result = get_default_persistence_dir()
assert result == new_dirThe screenshots prove manual testing works, but automated tests ensure this doesn't regress when V0 support is eventually removed.
mamoodi
pushed a commit
that referenced
this pull request
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of PR
We now support the legacy
FILE_STORE_PATHvariable when identifying the persistence directory. We will swap over to the new variable when we remove support for V0 - for now it is still defined in the Dockerfile.Demo Screenshots/Videos
Change Type
Checklist
Fixes
#13578
Resolves #(issue)
Release Notes