-
Notifications
You must be signed in to change notification settings - Fork 20
fix(bluefin, devmode): correctly rebase people to (image)-dx-nvidia-open #157
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
fix(bluefin, devmode): correctly rebase people to (image)-dx-nvidia-open #157
Conversation
This also adds a lil notice for using dx-group after reboot Signed-off-by: Tulip Blossom <tulilirockz@outlook.com>
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.
Code Review
This pull request fixes an issue with rebasing to developer-mode images for variants like nvidia, by correctly constructing the target image name. It also improves the user experience by clarifying instructions and ensuring developer flatpaks are installed correctly when switching. The changes are logical and address the problem described. However, I've identified a related high-severity issue where detecting an existing developer image will fail for these new image name variants, preventing users from switching back. This should be addressed to make the feature fully functional.
| IMAGE_NAME="$(jq -rc '."image-name"' "${IMAGE_INFO_FILE}")" | ||
| IMAGE_REF="$(jq -rc '."image-ref"' "${IMAGE_INFO_FILE}" | sed "s/.*ghcr/ghcr/")" | ||
| CURRENT_IMAGE="${IMAGE_REF}:$(jq -rc '."image-tag"' "${IMAGE_INFO_FILE}")" | ||
| IMAGE_BASE_NAME="$(cut -f1 -d\- <<< "${IMAGE_NAME}")" |
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.
This change correctly determines the base image name, which is necessary for handling image variants like bluefin-nvidia.
However, this change reveals a significant issue in the logic for detecting if the system is already in developer mode. The check on line 26 (if grep -q -E -e "dx$" <<< "${IMAGE_NAME}") only looks for dx at the very end of the image name.
With this PR, it's now possible to rebase to an image named something like bluefin-dx-nvidia. For such an image, the check on line 26 will fail. This means a user on a ...-dx-nvidia image will be unable to switch back to a non-developer image using this script, as it will incorrectly offer to enable developer mode again.
To ensure the toggle works both ways, the check should be updated to find the -dx infix. For example:
if grep -q -- "-dx" <<< "${IMAGE_NAME}"; thenWhile this line is outside the diff, this is a critical bug that this PR's changes will expose to users. It should be addressed for the devmode toggle to be reliable.
|
Hello, I have reviewed the changes in #157 which were intended to address the However, based on my testing:
failed to invoke method OpenImage
This indicates that the change in PR 157 does not fully resolve the issue in at least the following scenarios:
Thus, although PR 157 is present in the codebase, the underlying GHCR authentication problem still blocks Please let me know if you would like additional logs or tests of specific commands. Thanks! |
This also adds a lil notice for using dx-group after reboot
Should fix: ublue-os/bluefin#4001