store/onprem: add .cache to .state migration#4615
Open
croissanne wants to merge 1 commit into
Open
Conversation
This was backported to 10.2. For certainty, let's keep it around for a bit longer, in case users upgrade from 10.0 or 10.1 straight to a future minor version.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider guarding the migration script with more explicit error handling or logging so failures copying from ~/.cache are visible and don't silently proceed to use an empty state directory.
- The inline comment about dropping after 10.4 is vague; it may be clearer to link this to a specific cleanup ticket or configuration to avoid leaving stale compatibility code indefinitely.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider guarding the migration script with more explicit error handling or logging so failures copying from ~/.cache are visible and don't silently proceed to use an empty state directory.
- The inline comment about dropping after 10.4 is vague; it may be clearer to link this to a specific cleanup ticket or configuration to avoid leaving stale compatibility code indefinitely.
## Individual Comments
### Comment 1
<location path="src/store/api/backend/onprem/composerApi/helpers/getBlueprintsPath.ts" line_range="16" />
<code_context>
}
const blueprintsDir = path.join(stateDir, BLUEPRINTS_DIR);
+ // Backwards compatibility, drop after 10.4?
+ await cockpit.script(`
+if [ ! -e "${blueprintsDir}" ] && [ -d "${user.home}/.cache/cockpit-image-builder" ] ; then
</code_context>
<issue_to_address>
**suggestion:** The version-specific backwards-compatibility note could become stale without a more explicit mechanism.
Because this cleanup is only indicated by a comment, it depends on someone remembering to remove it. Consider driving this via a feature flag or a version check, or at least replace the question mark with a clear TODO that references the cutoff version so it’s easier to track and remove when the time comes.
Suggested implementation:
```typescript
const blueprintsDir = path.join(stateDir, BLUEPRINTS_DIR);
// TODO: Remove this backwards-compatibility shim after cockpit-image-builder version 10.4.
// When dropping support for pre-10.4, delete this block and the legacy cache directory handling.
await cockpit.script(`
if [ ! -e "${blueprintsDir}" ] && [ -d "${user.home}/.cache/cockpit-image-builder" ] ; then
mkdir -p "${stateDir}"
cp -a "${user.home}/.cache/cockpit-image-builder" "${stateDir}/"
fi
`);
```
If the project introduces a feature flag or a version check mechanism for compatibility shims elsewhere in the codebase, this block should also be guarded by that mechanism (for example, checking the cockpit-image-builder version or a configuration flag) so it can be disabled centrally when support for versions before 10.4 is dropped.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4615 +/- ##
==========================================
- Coverage 75.90% 71.53% -4.37%
==========================================
Files 225 255 +30
Lines 7209 6830 -379
Branches 2668 2503 -165
==========================================
- Hits 5472 4886 -586
- Misses 1494 1911 +417
+ Partials 243 33 -210
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Collaborator
|
Looks fine, just need #4616 to land first |
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
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.
This was backported to 10.2. For certainty, let's keep it around for a bit longer, in case users upgrade from 10.0 or 10.1 straight to a future minor version.