admin/pin: Add commands to pin booted, pending and rollbacks deployments #3146
Conversation
|
Linked to #3144 , must complete man pages. |
ericcurtin
force-pushed
the
ot-admin-builtin-pin-booted-pending-rollback
branch
from
8c17329 to
48a03c7
Compare
|
@cgwalters ok, I'll just change it to a string comparison, it will be 95% the same code |
ericcurtin
force-pushed
the
ot-admin-builtin-pin-booted-pending-rollback
branch
from
48a03c7 to
6b4415a
Compare
|
This one is ready for review |
ericcurtin
force-pushed
the
ot-admin-builtin-pin-booted-pending-rollback
branch
from
6b4415a to
9db5d13
Compare
src/ostree/ot-admin-builtin-pin.c
Outdated
| for (gint i = 0; i < deployments->len; ++i) | ||
| if (deployments->pdata[i] == target_deployment) |
There was a problem hiding this comment.
BTW our style generally prefers braces { in these types of cases, really braceless is only for a simple if, but this is irrelevant per above
There was a problem hiding this comment.
Add { } to both for and if I assume?
There was a problem hiding this comment.
The sort of vague unspecified rule I follow is that if you have any conditional/looping instructions that nest, then use braces at least on the outer one.
So
for (...) {
if (something)
return i;
}
is OK too. The rationale behind this is the same as that for -Wmisleading-indentation.
src/ostree/ot-admin-builtin-pin.c
Outdated
| return glnx_throw (error, "Invalid index: %s", deploy_index_str); | ||
| if (errno == ERANGE) | ||
| { | ||
| if (!g_strcmp0 (deploy_index_str, "booted")) |
There was a problem hiding this comment.
Hmm I'd say we should check the string values first, and then only fall back to trying to parse as an integer.
src/ostree/ot-admin-builtin-pin.c
Outdated
| deploy_index = get_deployment_index_for_type (sysroot, OSTREE_DEPLOYMENT_PENDING); | ||
| else if (!g_strcmp0 (deploy_index_str, "rollback")) | ||
| deploy_index = get_deployment_index_for_type (sysroot, OSTREE_DEPLOYMENT_ROLLBACK); | ||
|
|
There was a problem hiding this comment.
Feels like we should do something to explicitly handle the notfound aka -1 case here? Like:
if (deploy_index < 0)
return glnx_throw (error, "Deployment type not found: %s", deploy_index_str);
There was a problem hiding this comment.
It's already handled inside ot_admin_get_indexed_deployment function, so I didn't worry about handling it again, but we can handle it here also if needs be.
There was a problem hiding this comment.
That will tell the user error: Invalid index -1 which seems much less friendly than error: Deployment type not found: rollback right?
ericcurtin
force-pushed
the
ot-admin-builtin-pin-booted-pending-rollback
branch
2 times, most recently
from
dc683c1 to
c969a3b
Compare
|
Tested and ready for review again |
Add new commands to pin the current, staged and previous deployment for use in automation and scripting. Right now, it's difficult to pin the current deployment without needing to look into the output of some other tooling (like rpm-ostree) to get the index of each deployment. This index also is not consistent - the current deployment could be 0 when you first boot the system then 1 shortly after. This change makes it easy to pin the current or future deployment. Co-authored-by: Robert Sturla <[email protected]> Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
ot-admin-builtin-pin-booted-pending-rollback
branch
from
c969a3b to
cda5103
Compare
|
Looks like an unrelated build failure maybe 🤷 |
|
For some reason |
|
@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge 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. |
Add new commands to pin the current, staged and previous deployment for
use in automation and scripting. Right now, it's difficult to pin the
current deployment without needing to look into the output of some other
tooling (like rpm-ostree) to get the index of each deployment. This
index also is not consistent - the current deployment could be 0 when
you first boot the system then 1 shortly after. This change makes it
easy to pin the current or future deployment.
Co-authored-by: Robert Sturla [email protected]
Signed-off-by: Eric Curtin [email protected]