Created attachment 277626 [details] [review]
deploy: Remove legacy "current" symbolic links

Might break some people's scripts, but they shouldn't be relying on this anyways.

Attachment 277626 [details] pushed as dfeb27e - deploy: Remove legacy "current" symbolic links

This looks like it will break the gnome-hwtest install script (not immediately, since the image isn't rebuilt unless manually retagged, and anyways this is just the script to install a new image.)

It's used to find out the current revision of a sysroot:

    ref=`readlink /ostree/deploy/gnome-continuous/current | sed 's@deploy/\([0-9a-f]*\).*@\1@'`

And to refer to the /etc currently deployed in a sysroot:

    fstab=/tmp/installroot/ostree/deploy/gnome-continuous/current/etc/fstab

What are the approved replacements?

(In reply to comment #4)

> What are the approved replacements?

The most powerful solution is to use libostree, e.g.:

    let sysroot = OSTree.Sysroot.new(mntdir);
    sysroot.load(null);
    let deployments = sysroot.get_deployments();
    let current = deployments[0];
    let deployDir = sysroot.get_deployment_directory(current);

However, we could add something to be convenient for shell scripts...

# ostree admin --sysroot=/path/to/sysroot status --print-current-dir

?

Leaving aside writing my particular scripts to JS (not inclined at the moment), I think being sysadmin scripting friendly is definitely a good thing.

--print-current-dir as a global option, or have a subcommand?

What about finding the current version, either of:

 csum=`ostree admin --print-current-dir| sed 's@deploy/\([0-9a-f]*\).*@\1@'`

 csum=`ostree admin status | while read current osname csum  ; do [ "$current" = '*' ] && echo $csum ; done`

seem clunky.

What did you want?  The tree revision (checksum)?  The deployment root?  (Note the latter may not be $checksum.0)

I separately have need for the deployment root and the current tree revision. 

Admittedly, the need for the current tree revision is a bit obscure - what I'm doing is doing a filesystem-level mirror of an already installed tree to a new partition, then rerunning 'ostree admin deploy' to change the bootloader config to have different --karg=root=LABEL=<blah>.

Since ostree admin deploy needs a ref, I need to figure out what the ref is - it may be fine to do one of the above hacks in this specialized case. Or maybe 'ostree admin deploy' needs a --refresh option that redeploys with the same tree revision.

Note for changing the kernel arguments, there is "ostree admin instutil set-kargs", which is almost what you want.  Would just need --karg-extend --karg-replace=, like deploy has.

Created attachment 287114 [details] [review]
ostree admin instutil set-kargs: make more flexible

Add command line arguments:
 --import-proc-cmdline: import values from /proc/cmdline
 --merge: import current values
 --replace=ARG=VALUE: replace value
 --append=ARG=VALUE: append a new argument

Extra command line arguments are treated like --append=, which
gives backwards compatibility.

Created attachment 287115 [details] [review]
ostree admin: Add a --print-current-dir option

Add an option --print-current-dir that prints the current deployment
directory to stdout and exits.

Review of attachment 287115 [details] [review]:

A little awkward that this is special cased instead of being another builtin.  OTOH, I know adding builtins is a lot of boilerplate.

And there is precedent in e.g. "git rev-parse --show-toplevel".  (What does telling me the the git topdir have to do with parsing revs??)

So...marking ACN.  But maybe we can think about some better way to handle frequently used vs infrequently used tools.  That's kind
of what I was going for with "instutil", but this isn't related to installing.

Review of attachment 287114 [details] [review]:

Looks good!  Would like some tests though.

Created attachment 287187 [details] [review]
Add test case for 'admin instutil set-kargs'

Test out the newly added options to 'instutil set-kargs' along with
the existing functionality.

Created attachment 287188 [details] [review]
Test 'ostree admin --print-current-dir'

Add a test for the --print-current-dir option

(In reply to comment #12)
> Review of attachment 287115 [details] [review]:
> 
> A little awkward that this is special cased instead of being another builtin. 
> OTOH, I know adding builtins is a lot of boilerplate.
> 
> And there is precedent in e.g. "git rev-parse --show-toplevel".  (What does
> telling me the the git topdir have to do with parsing revs??)
> 
> So...marking ACN.  But maybe we can think about some better way to handle
> frequently used vs infrequently used tools.  That's kind
> of what I was going for with "instutil", but this isn't related to installing.

I just literally followed what you suggested in comment #5 without thinking about it too much. I was also reminded of the git rev-parse option; at least we're putting this on 'ostree admin' not on a random subcommand, so there might be some chance of it being found...

Review of attachment 287187 [details] [review]:

LGTM

Review of attachment 287188 [details] [review]:

Yep.

(In reply to comment #16)

> 
> I just literally followed what you suggested in comment #5 without thinking
> about it too much. 

Right, forgot I said that =)

> I was also reminded of the git rev-parse option; at least
> we're putting this on 'ostree admin' not on a random subcommand, so there might
> be some chance of it being found...

Yep, I think it's fine.  Let's go with it.

Attachment 287114 [details] pushed as 262cba0 - ostree admin instutil set-kargs: make more flexible
Attachment 287115 [details] pushed as d64d003 - ostree admin: Add a --print-current-dir option
Attachment 287187 [details] pushed as c3f8019 - Add test case for 'admin instutil set-kargs'
Attachment 287188 [details] pushed as cc180f5 - Test 'ostree admin --print-current-dir'