-
Notifications
You must be signed in to change notification settings - Fork 305
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
Add commands to get index of booted, pending and previous deployments #3144
Conversation
Hi @p5. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
278b1b3
to
15f4dba
Compare
Will need equivalent man page updates too. You can also collapse those 3 else if's into ingel return statements if you write a function that takes two parameters. I'm in bed now but I can write the function signature in the morning to point you in the right direction. |
Noticed your comment about how to build... I use this: |
I believe I have done this: I will mark this as draft for now as there's a high chance I have messed something up and want to test this out locally. But please let me know if the refactoring is not what you intended. |
Ah, just found you already upload built binaries during CI, so that makes testing these changes locally so much easier! Command outputs:
RPM Ostree Status:
Man page:
Now I've confirmed it seems to work, the only remaining things are to find out how you intended the refactoring to look and for me to squash all commits. |
After you build locally, you can test from the git repo using commands like:
I pulled the branch down, de-duplicated the code further, made some other styling changes to be consistent with the rest of the project (we don't do camelCase for example for variable names, although types have it). I squashed the commits. Fixed up the commit message (we put "status: " for example in the commit message when we change this part). The angular brackets were missing in the Co-authored-by for example, if we push this, we should be pretty good: |
Also had to remove changes accidentally made to libglnx, that may have been my mistake, unsure... |
7b60e5f
to
5242e56
Compare
Add new commands to output 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 get the index of the current or future deployment so it can be piped into other commands (such as ostree admin pin). Huge thanks to Eric for authoring this patch for me! Co-authored-by: Eric Curtin <[email protected]> Signed-off-by: Robert Sturla <[email protected]>
5242e56
to
22d5602
Compare
All changes have been made. I'm not sure where "changes accidentally made to libglnx" could be found in your commit, so if I have missed something, please let me know. And sorry it's taken so many revisions to get something that's in a good enough state. I'm not familiar at all with C so it was very much trial and error to get something that seemed right and CI didn't shout at. |
Well you did a pretty amazing job at it for someone that doesn't know C! 😄 This LGTM, once Draft is removed I think we can merge unless another ostree maintainer wants to look at this also in the next 24 hours. |
As an aside, I'm looking for someone in the community with cycles and an Apple Silicon device to look at this: ericcurtin/fedora-asahi-ostree-desktops#1 if you know of someone that wants to look at this let me know. It's hard for me to justify looking at it, as my Apple Silicon device is my daily driver and it blocks my Automotive work when I start flashing and rebooting it to test Asahi Kinoite/Silverblue. |
/ok-to-test |
Not strongly against, though these options seem somewhat obscure. I think probably what we want instead is e.g. That said, note you can also do something like |
Closes #3142
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 get the index of the current or future deployment so it can be piped into other commands (such as ostree admin pin).
(Please note: I have not yet built or tested this PR - I am working on figuring out how to compile it)
Huge thanks to Eric for authoring this patch for me!
TODO before merge:
Co-authored-by: Eric Curtin [email protected]