-
Notifications
You must be signed in to change notification settings - Fork 290
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
grub2: Generate config from pending deployment #1831
grub2: Generate config from pending deployment #1831
Conversation
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.
Something is going wrong in PAPR I think, but let's get the tests passing. Otherwise besides one minor nit, LGTM!
Taking a look at the failing CI test, thanks! |
Split the creation of the directory containing grub.cfg, and the creation of the file, so that a failure in the mkdir command will fail the test and not attempt the touch command.
16df565
to
5fd06d6
Compare
98b47c4
to
5fd06d6
Compare
Reworked the test so that it exercises the However, the test runs into diff --git a/src/libostree/ostree-bootloader-grub2.c b/src/libostree/ostree-bootloader-grub2.c
index 1e5791d65..7feba43a2 100644
--- a/src/libostree/ostree-bootloader-grub2.c
+++ b/src/libostree/ostree-bootloader-grub2.c
@@ -321,7 +321,7 @@ grub2_child_setup (gpointer user_data)
_exit (1);
}
- if (chroot (".") != 0)
+ if (chroot ("/") != 0)
{
perror ("chroot");
_exit (1); Manually testing in Fedora Atomic Host 29 with
(previously the above command hit the assertion in #1774). The tests in To have the test run
grub2-mkconfig installed on the host running the test.
Considering manually testing the changes here works though, would the libostree changes be OK to merge and then the automated test can be dealt with separately? Also see there is some history here: https://bugzilla.gnome.org/show_bug.cgi?id=761180 |
5fd06d6
to
f037b5e
Compare
For now I just removed the auto test - as it seems the This is the test trying to exercise diff --git a/tests/test-admin-deploy-grub2.sh b/tests/test-admin-deploy-grub2.sh
index 231aa2f8..f3e2180a 100755
--- a/tests/test-admin-deploy-grub2.sh
+++ b/tests/test-admin-deploy-grub2.sh
@@ -26,6 +26,21 @@ set -euo pipefail
# Exports OSTREE_SYSROOT so --sysroot not needed.
setup_os_repository "archive" "grub2 ostree-grub-generator"
-extra_admin_tests=0
+extra_admin_tests=1
. $(dirname $0)/admin-test.sh
+
+# Check that deploying a new tree with zero previous deployments works.
+# (see https://github.com/ostreedev/ostree/issues/1774).
+cd ${test_tmpdir}
+rm httpd osdata testos-repo sysroot -rf
+setup_os_repository "archive" "grub2"
+# Exercise the system grub2-mkconfig codepath.
+export OSTREE_GRUB2_EXEC=grub2-mkconfig
+${CMD_PREFIX} ostree pull-local --repo=sysroot/ostree/repo --remote testos testos-repo testos/buildmaster/x86_64-runtime
+${CMD_PREFIX} ostree admin deploy --karg=root=LABEL=MOO --karg=quiet --sysroot sysroot --os testos testos/buildmaster/x86_64-runtime > out.txt
+assert_file_has_content out.txt "Transaction complete.*"
+assert_file_has_content sysroot/boot/loader/entries/ostree-1-testos.conf 'options.* root=LABEL=MOO'
+assert_file_has_content sysroot/boot/ostree/testos-${bootcsum}/vmlinuz-3.6.0 'a kernel'
+assert_file_has_content sysroot/boot/ostree/testos-${bootcsum}/initramfs-3.6.0.img 'an initramfs'
+echo "ok run grub2 bootloader on first deployment" |
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 looks good to me! Just one super minor/optional nit.
Generate a grub2 config using the pending deployment, if a grub2 bootloader is detected in the sysroot. Allows grub2-mkconfig to run if there are no previous deployments. Fixes: ostreedev#1774
f037b5e
to
dc2f6c0
Compare
Thanks! Re. tests, yeah I don't think there's an easy way to add this to the |
⚡ Test exempted: merge already tested. |
Split the creation of the directory containing grub.cfg, and the creation of the file, so that a failure in the mkdir command will fail the test and not attempt the touch command. Closes: #1831 Approved by: jlebon
Attempt at a fix for #1774. Previously pushed these changes (see #1774 (comment)) - posting a PR so we can discuss here.
Involves an interface change to add the
new_deployments
argument to each bootloader*_write_config
function. It'd be preferable to avoid passing down the extra argument - but this seems to be what's needed to expose the pending deployment to the bootloader functions.Need to verify that the tests work (recently updated my dev environment and tests that previously passed are failing locally).