Revert grub2 exit #1955
Revert grub2 exit #1955
Conversation
|
/cc @martinezjavier |
|
@cgwalters: GitHub didn't allow me to request PR reviews from the following users: martinezjavier. Note that only ostreedev members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@cgwalters I just had a trivial comment, but other than that I agree with your changes. |
What's the comment? I don't see it here? |
src/boot/grub2/grub2-15_ostree
Outdated
| # commit 985a14100295c99d0c6d712bfbee0ec52a3a1601 | ||
| # https://discussion.fedoraproject.org/t/boot-entries-gone-after-upgrade/8026/3 | ||
| # https://github.com/ostreedev/ostree/pull/1929 | ||
| if test -f /boot/.grub2-bls-enabled; then |
There was a problem hiding this comment.
Hmm, in the idea I had in #1929 (comment), I suggested checking for both GRUB_ENABLE_BLSCFG and a stamp file that that means "this bootloader supports blscfg" (so probably should be named e.g. .grub2-bls-supported).
With this approach, if someone turns off GRUB_ENABLE_BLSCFG again, we go back to no entries. Not that folks would normally want to do that, but seems worth handling it given the consequences. (One case that comes to mind for example is someone rolling back to a previous deployment that doesn't enable GRUB_ENABLE_BLSCFG.)
There was a problem hiding this comment.
With this approach, if someone turns off
GRUB_ENABLE_BLSCFGagain, we go back to no entries. Not that folks would normally want to do that, but seems worth handling it given the consequences. (One case that comes to mind for example is someone rolling back to a previous deployment that doesn't enableGRUB_ENABLE_BLSCFG.)
@jlebon I thought that @cgwalters idea here was to not allow the user to choose but instead let the installer / OS to specify whether the installed bootloader has BLS support or not and just use it if that's the case.
That way you won't use a config option that's specific to Fedora GRUB but instead a protocol that's defined by OSTree that bootloaders can use to specify that are able to parse BLS files.
But you are right too that maybe users should be allowed to disable the option.
Probably this PR should be split in two, so the revert can be merged to drop the downstream patch while the second commit is discussed.
src/boot/grub2/grub2-15_ostree
Outdated
| # commit 985a14100295c99d0c6d712bfbee0ec52a3a1601 | ||
| # https://discussion.fedoraproject.org/t/boot-entries-gone-after-upgrade/8026/3 | ||
| # https://github.com/ostreedev/ostree/pull/1929 | ||
| if test -f /boot/.grub2-bls-enabled; then |
There was a problem hiding this comment.
Any reason why we would want the filename to be grub2 specific?
I think that we could just have a generic .bootloader-bl-enable or .bls-enabled, that could potentially used be used also by other bootloaders / installers to indicate OSTree that the installed bootloader is capable of parsing BLS snippets.
Sorry, it's now. I noticed that never submitted my comment. |
cgwalters
force-pushed
the
revert-grub2-exit
branch
from
69e0b9b
to
27f388d
Compare
This reverts commit 985a141. It turned out that some people have old bootloaders, and hence get the "no entries" problem. That's much, much much worse than double entries.
cgwalters
force-pushed
the
revert-grub2-exit
branch
from
27f388d
to
650d625
Compare
|
Updated the PR to just revert |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
bot, retest this please |
|
/override f29-flatpak |
|
@cgwalters: Overrode contexts on behalf of cgwalters: f29-flatpak In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
cgwalters
changed the title
In this case this is reverted in Fedora; I don't like carrying downstream patches. Let's revert it here, and I added a different approach.