Add pending-base-commit to status
#595
Conversation
One thing that's very confusing about OSTree is there are two layers - deployments and the refs/commits. If one does an `rpm-ostree upgrade`, but then e.g. `ostree admin undeploy 0`, you still have the new revision in the repo. We don't do a good job of displaying this state, or helping people clean it up. Down the line, I also want to better support something like `rpm-ostree pull` to cache updates explicitly *without* deploying. This commit just adds a bit of information to the status display. We might want to have better formatting, but I think this an OK start.
|
This should be good now, the compose test just flaked on repodata. |
There was a problem hiding this comment.
So the assumption here is that the refspec is currently pointing at the base commit we want to deploy? Though in the case of rebase, that doesn't hold true.
One thing that's very confusing about OSTree is there are two layers -
deployments and the refs/commits. If one does anrpm-ostree upgrade, but then
e.g.ostree admin undeploy 0, you still have the new revision in the repo.
But then when the user does rpm-ostree upgrade again, we'll still contact the remote, right? So then that pending commit might not be what we actually use anyway.
| csum, &gpg_enabled); | ||
| { | ||
| base_checksum = csum; | ||
| sigs = rpmostreed_deployment_gpg_results (repo, refspec, csum, &gpg_enabled); |
There was a problem hiding this comment.
Looks like you can just hoist this out of the if and else blocks.
src/app/rpmostree-builtin-status.c
Outdated
| { | ||
| const char *base_checksum; | ||
| g_assert (g_variant_dict_lookup (dict, "base-checksum", "&s", &base_checksum)); | ||
| print_kv ("BaseCommit", max_key_len, base_checksum); | ||
| } | ||
| print_kv ("Commit", max_key_len, checksum); | ||
|
|
||
| /* Pending state, but only for the booted comit */ |
| variant_add_commit_details (&dict, NULL, commit); | ||
|
|
||
| if (!ostree_repo_resolve_rev (repo, refspec, TRUE, | ||
| &pending_base_commitrev, error)) |
There was a problem hiding this comment.
This won't work with rebases though, right?
There was a problem hiding this comment.
What I would say here is that at the moment, the data I'm trying to add is just looking for any changes in the origin refspec vs deployed checksum - for each deployment independently.
So it "works" with rebases by ignoring it, because it's a new deployment. I wouldn't say it's broken, but it's showing you information you don't need. Admittedly it is a bit redundant too if you do an atomic host upgrade; atomic host status.
|
Maybe to be least confusing, I can only show the information if there isn't a pending deployment. In that case actually, we'd only show it if the user explicitly did an |
|
Ok, did that in ⬆️ |
|
OK, that works! |
|
☀️ Test successful - status-atomicjenkins |
Commands like `upgrade` and `deploy` need to know if a new deployment was actually laid down so that it may print a pkg diff if so. This is implemented by listening for changes to the DefaultDeployment D-Bus property. D-Bus emits a signal when the deployment variant changes value. However, in coreos#595, with the introduction of `pending-*` related keys, the deployment variant no longer represents data solely tied to that specific deployment. In this case, because `deploy` operations currently set the ref to the resolved checksum, it can happen that deploying the same base commit when the current refspec *isn't* pointing to that base commit will result in the `pending-*` keys dropping out and a default deployment change notification going out. In this patch, we strengthen how we determine whether a new deployment was laid down by actually looking at the deployment id, rather than just assuming that a change to the property implies a new deployment. Closes: coreos#981
Commands like `upgrade` and `deploy` need to know if a new deployment was actually laid down so that it may print a pkg diff if so. This is implemented by listening for changes to the DefaultDeployment D-Bus property. D-Bus emits a signal when the deployment variant changes value. However, in #595, with the introduction of `pending-*` related keys, the deployment variant no longer represents data solely tied to that specific deployment. In this case, because `deploy` operations currently set the ref to the resolved checksum, it can happen that deploying the same base commit when the current refspec *isn't* pointing to that base commit will result in the `pending-*` keys dropping out and a default deployment change notification going out. In this patch, we strengthen how we determine whether a new deployment was laid down by actually looking at the deployment id, rather than just assuming that a change to the property implies a new deployment. Closes: #981 Closes: #984 Approved by: cgwalters
One thing that's very confusing about OSTree is there are two layers -
deployments and the refs/commits. If one does an
rpm-ostree upgrade, but thene.g.
ostree admin undeploy 0, you still have the new revision in the repo.We don't do a good job of displaying this state, or helping people clean
it up.
Down the line, I also want to better support something like
rpm-ostree pulltocache updates explicitly without deploying.
This commit just adds a bit of information to the status display. We might want
to have better formatting, but I think this an OK start.