-
Notifications
You must be signed in to change notification settings - Fork 307
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
Allow commits to mark refs as EOL, replaced by others #874
Conversation
This will be used by the upcoming test-admin-upgrade-endoflife.sh
Can one of the admins verify this patch?
|
bot, add author to whitelist |
src/libostree/ostree-repo.c
Outdated
@@ -3526,7 +3526,7 @@ ostree_repo_read_commit (OstreeRepo *self, | |||
gboolean | |||
ostree_repo_pull (OstreeRepo *self, | |||
const char *remote_name, | |||
char **refs_to_fetch, | |||
const char **refs_to_fetch, |
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.
We can't do this with a stable API; people turn these sorts of warnings into errors for good reason.
In the "GLib-and-above" stack, we're not very consistent about this. The strictly correct thing is more const char * const*
, which is long and annoying to type. It's used by e.g. g_variant_new_strv()
. But not for example g_strdupv()
.
Anyways, again we can't change our API, so please rebase without this.
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.
(And we just have to eat the pain of casts between char**
and const char * const*
).
@@ -512,6 +512,10 @@ ostree_sysroot_upgrader_pull_one_dir (OstreeSysrootUpgrader *self, | |||
const char *refs_to_fetch[] = { NULL, NULL }; | |||
const char *from_revision = NULL; | |||
g_autofree char *origin_refspec = NULL; | |||
g_autofree char *new_revision; |
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.
Init everything that uses autoptr to NULL
for refactoring safety.
|
||
g_variant_get_child (new_variant, 0, "@a{sv}", &new_metadata); | ||
rebase = g_variant_lookup_value (new_metadata, "ostree.endoflife-rebase", G_VARIANT_TYPE_STRING); | ||
if (rebase) { |
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.
Braces on newline (GNU style).
Code seems reasonable offhand. One note to self is that |
A commit can now include a "ostree.endoflife-rebase" metadata key pointing to a new ref. When updating, the sysroot upgrader will see this and proceed to pull and deploy the new ref instead. The origin file in the new deployment will point to the new ref. This functionality is planned to be used in Endless OS. We will create a lesser tested branch for brand new, cutting edge hardware support, and ship that on hardware platforms that require the latest drivers. However, once our slower-moving official release is later updated to support the new hardware, we will use this functionality to migrate those bleeding-edge users over to the official release.
I rebased to remove the char API change, and pushed a new commit also addressing the other review comments. |
g_free (self->origin_ref); | ||
self->origin_ref = g_strdup(new_ref); | ||
g_free (origin_refspec); | ||
origin_refspec = g_strconcat (self->origin_remote, ":", new_ref, NULL); |
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.
We have to account for local branches here too, right?
E.g. something like
if (self->origin_remote)
origin_refspec = g_strconcat (self->origin_remote, ":", new_ref, NULL);
else
origin_refspec = g_strdup (new_ref);
?
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.
Or possibly just do the whole block under if (rebase && self->origin_remote)
? I can't think of a use case for local refs being redirected. But there's also an argument that we should be symmetric in our handling of local/remote refs - your change is definitely right from that perspective.
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.
The symmetry argument definitely appeals to me, yeah. Even if only for making it easier to play with and test locally.
I pushed another fixup commit which adds @jlebon's suggestion for handling local branches. |
Thanks! |
A commit can now include a "ostree.endoflife-rebase" metadata key pointing to a new ref. When updating, the sysroot upgrader will see this and proceed to pull and deploy the new ref instead. The origin file in the new deployment will point to the new ref. This functionality is planned to be used in Endless OS. We will create a lesser tested branch for brand new, cutting edge hardware support, and ship that on hardware platforms that require the latest drivers. However, once our slower-moving official release is later updated to support the new hardware, we will use this functionality to migrate those bleeding-edge users over to the official release. Closes: #874 Approved by: cgwalters
☀️ Test successful - status-atomicjenkins |
A commit can now include a "ostree.endoflife-rebase" metadata key
pointing to a new ref.
When updating, the sysroot upgrader will see this and proceed to
pull and deploy the new ref instead. The origin file in the new
deployment will point to the new ref.
This functionality is planned to be used in Endless OS. We will create
a lesser tested branch for brand new, cutting edge hardware support,
and ship that on hardware platforms that require the latest drivers.
However, once our slower-moving official release is later updated to
support the new hardware, we will use this functionality to migrate
those bleeding-edge users over to the official release.