ostree: Describe subcommands in help output #1267

peterbaouoft · 2017-10-13T17:57:41Z

The added subcommands help will be provided to the following commands:

ostree -h
ostree admin -h
ostree admin instutil -h
ostree remote -h
ostree static-delta -h

This is very similar to coreos/rpm-ostree#916

Closes: #1163
Demo:

[root@localhost ostree]# ostree -h
Usage:
  ostree [OPTION?] COMMAND

Builtin Commands:
  admin             Commands that needs admin privilege
  cat               Concatenate contents of files
  checkout          Check out a commit into a filesystem tree
  checksum          Checksum a file or directory
  commit            Commit a new revision
  config            Change repo configuration settings
  diff              Compare directory TARGETDIR against revision REV
  export            Stream COMMIT to stdout in tar format
  fsck              Check the repository for consistency
  gpg-sign          Sign a commit
  init              Initialize a new empty repository
  log               Show log starting at commit or ref
  ls                List file paths
  prune             Search for unreachable objects
  pull-local        Copy data from SRC_REPO
  pull              Download data from remote repository
  refs              List refs
  remote            Remote commands that may involve internet access
  reset             Reset a REF to a previous COMMIT
  rev-parse         Output the target of a rev
  show              Output a metadata object
  static-delta      Static delta related commands
  summary           Manage summary metadata

Help Options:
  -h, --help        Show help options

Application Options:
  -v, --verbose     Print debug information during command processing
  --version         Print version information and exit

[root@localhost ostree]# ostree admin -h
Usage:
  ostree admin [OPTION?] --print-current-dir|COMMAND

Builtin "admin" Commands:
  cleanup            Delete untagged deployments and repository objects
  config-diff        Diff current /etc configuration versus default
  deploy             Checkout revision REFSPEC as the new default deployment
  init-fs            Initialize a root filesystem
  instutil           Provide instutil commands, allow admin to change boot configuration and relabel selinux 
  os-init            Initialize empty state for given operating system
  set-origin         Set Origin and create a new origin file
  status             List deployments
  switch             Construct new tree from REF and deploy it
  undeploy           Delete deployment INDEX
  unlock             Make the current deployment mutable (as a hotfix or development)
  upgrade            Construct new tree from current origin and deploy it, if it changed

Help Options:
  -h, --help         Show help options

Application Options:
  --sysroot=PATH     Create a new OSTree sysroot at PATH
  -v, --verbose      Print debug information during command processing
  --version          Print version information and exit

[root@localhost ostree]# ostree admin instutil -h
Usage:
  ostree admin instutil [OPTION?] COMMAND

Builtin "admin instutil" Commands:
  selinux-ensure-labeled  relabel all or part of a deployment
  set-kargs               set new kernel command line arguments(Not stable) 
  grub2-generate          generate GRUB2 configuration from given BLS entries

Help Options:
  -h, --help         Show help options

Application Options:
  --sysroot=PATH     Create a new OSTree sysroot at PATH
  -v, --verbose      Print debug information during command processing
  --version          Print version information and exit

[root@localhost ostree]# ostree remote -h        
Usage:
  ostree remote [OPTION?] COMMAND

Builtin "remote" Commands:
  add               Add a remote repository
  delete            Delete a remote repository
  show-url          Show remote repository URL
  list              List remote repository names
  gpg-import        Import GPG keys
  add-cookie        Add a cookie to remote
  delete-cookie     Remote one cookie from remote
  list-cookies      Show remote repository cookies
  refs              List remote refs
  summary           Show remote summary

Help Options:
  -h, --help        Show help options

Application Options:
  -v, --verbose     Print debug information during command processing
  --version         Print version information and exit

[root@localhost ostree]# ostree static-delta -h
Usage:
  ostree static-delta [OPTION...] COMMAND

Builtin "static-delta" Commands:
  list             list static delta files
  show             Dump information on a delta
  delete           Remove a delta
  generate         Generate static delta files
  apply-offline    Apply static delta file

peterbaouoft · 2017-10-13T18:29:35Z

Looks like... the failure was because... the test-help.sh file also reads description as one of the potential commands :(.

_fatal_print_file out 'File '''out''' doesn'''t match regexp '''ostree admin Delete''''

:'(.

jlebon · 2017-10-13T18:49:26Z

Ahh, heh. I think you might need to tweak the sed so it only picks up the first word of each line? Alternatively, you can also pipe into e.g. cut -d' '.

jlebon · 2017-10-13T18:51:34Z

src/ostree/ot-admin-builtin-instutil.c

 } OstreeAdminInstUtilCommand;

 static OstreeAdminInstUtilCommand admin_instutil_subcommands[] = {
 #ifdef HAVE_SELINUX
-  { "selinux-ensure-labeled", ot_admin_instutil_builtin_selinux_ensure_labeled },
+  { "selinux-ensure-labeled", ot_admin_instutil_builtin_selinux_ensure_labeled,
+    "relabel all or part of a deployment" },


Let's capitalize these too to be consistent.

jlebon · 2017-10-13T18:54:04Z

src/ostree/ot-admin-builtin-instutil.c

-  { "grub2-generate", ot_admin_instutil_builtin_grub2_generate },
-  { NULL, NULL }
+  { "set-kargs", ot_admin_instutil_builtin_set_kargs,
+    "set new kernel command line arguments(Not stable) "  },


Minor: trailing space there!

jlebon · 2017-10-13T18:58:22Z

src/ostree/ot-remote-builtin-delete-cookie.c

@@ -41,7 +41,7 @@ gboolean
 ot_remote_builtin_delete_cookie (int argc, char **argv, GCancellable *cancellable, GError **error)
 {
  g_autoptr(OstreeRepo) repo = NULL;
-  g_autoptr(GOptionContext) context = g_option_context_new ("NAME DOMAIN PATH COOKIE_NAME- Remote one cookie from remote");
+  g_autoptr(GOptionContext) context = g_option_context_new ("NAME DOMAIN PATH COOKIE_NAME- Remove one cookie from remote");


It looks good overall! The only issue I have is that the description string is now duplicated twice right? Once in the struct and once in the parameter string?

Maybe let's pass the struct to the command so that ostree_option_context_parse can set it as the summary? Similar to what we do in rpm-ostree.

yea, we could do like coreos/rpm-ostree@12c34bb.

But it kinda becomes tricky when we have multiple type of commands included in Ostree. One for OstreeAdmin, OstreeAdminInstutil, OstreeRemote, and things become a bit messier when we have two different parsing method. ostree_option_context_parse and ostree_admin_option_context_parse. So, going through a similar invocation line as rpm-ostree may involve many changes/refactoring to the existing code.

But, it may be not that difficult if we can merge/change all type of different commands in ostree into one whole OstreeCommand, and then passing the struct along won't be that hard. But that also involves changing a lot of code :(, since I am a newbie here, I am inexperienced about whether or not it is worth to do so :p. But I think for now will just go ahead and have it a try :)

jlebon · 2017-10-13T18:59:11Z

src/ostree/ot-builtin-static-delta.c

-  { "apply-offline", ot_static_delta_builtin_apply_offline },
-  { NULL, NULL }
+  { "list", ot_static_delta_builtin_list,
+    "list static delta files" },


s/list/List/

Added a description argument to all type of commands. Now when we include -h or --help for commands that contain subcommands, the description for those subcommands are shown. The added subcommands help will be provided to the following commands: - ostree -h - ostree admin -h - ostree admin instutil -h - ostree remote -h - ostree static-delta -h

This is a similar approach as coreos/rpm-ostree@12c34bb. One thing to note is when we parse the admin related functions, we still keep the old admin related flags, and added a new parameter to represent the command struct. This allows us to identify the caller of the function, making it easier for us to possibly deduplicate the subcommand handling in the future. A similar approach is done in rpm-ostree: coreos/rpm-ostree@83aeb01 This also makes it easier for us to change the prototype of the function. If we want to add something new in the future, we won't need to touch every prototype.

peterbaouoft · 2017-10-17T18:33:21Z

So Rebased... and addressed most of comments ⬆️.

description string is now duplicated twice right? Once in the struct and once in the parameter string

This patch will be the prep for that. Another patch will hopefully come soon :).

There are actually a lot of files being changed in order to be able to "pass the command struct" through builtins. Also, it is my first time doing this type of refactoring, so I might make mistakes and please tell me if I did it wrong. :)

So, for the most recent patch, there are a few things worth mentioning(I think):

1: The built in flags for admin related commands are 'OSTREE_BUILTIN_FLAG_NO_REPO', and the OstreeAdminBuiltinFlags is unchanged and remained the same. The ostree_admin_option_context_parse
will have one more parameter invocation to represent the command struct. This was just me .. trying to avoid changing too much of the code :p.

2: The default builtin flag in ostree_option_context_parse is OSTREE_BUILTIN_FLAG_NO_REPO, when no command struct is passed in.

3: The other changes are all very similar to that from rpm-ostree coreos/rpm-ostree@12c34bb

This is similar idea as coreos/rpm-ostree@5c0bf88, The duplicated description is now removed, and the description of the command is now displayed beneath the Usage. For example: ostree cat -h will output the following: "Usage: ostree cat [OPTION?] COMMIT PATH... Concatenate contents of files"

…tins

cgwalters · 2017-10-19T16:11:25Z

src/ostree/ot-main.c

+          g_autoptr(GString) description_string = g_string_new (invocation->command->description);
+
+          g_string_append (description_string, "\n\n");
+          g_string_prepend (new_summary_string, description_string->str);


Could avoid another GString allocation here with:

g_string_prepend (new_summary_string, "\n\n"); g_string_prepend (new_summary_string, invocation->command->description);

But eh. If you prefer as is that's OK by me too.

cgwalters · 2017-10-19T16:12:04Z

src/ostree/main.c

+   */
+  { "admin", OSTREE_BUILTIN_FLAG_NO_REPO,
+    ostree_builtin_admin,
+    "Commands that needs admin privilege" },


I'd say Commands for managing a host system booted with ostree or so.

cgwalters · 2017-10-19T16:12:56Z

src/ostree/main.c

+    "Commands that needs admin privilege" },
+  { "cat", OSTREE_BUILTIN_FLAG_NONE,
+    ostree_builtin_cat,
+    "Concatenate contents of files"},


Maybe Like Unix cat, but reads data from an OSTree repository ?

Hum...perhaps we shouldn't be rewriting the descriptions in this commit. Nevermind, let's do that after.

cgwalters · 2017-10-19T16:13:54Z

Is this still WIP? Looks pretty good to me.

peterbaouoft · 2017-10-19T16:16:31Z

Is this still WIP? Looks pretty good to me.

Not any more :), I just updated the title, but unable to remove the label. And thanks for review, will address the comments shortly :).

peterbaouoft · 2017-10-19T16:36:36Z

Maybe Like Unix cat, but reads data from an OSTree repository ?
Hum...perhaps we shouldn't be rewriting the descriptions in this commit. Nevermind, let's do that after.

Sure :), added fixup based on comments above ⬆️

cgwalters · 2017-10-20T12:59:30Z

Nice work, thanks a lot!

@rh-atomic-bot r+ 6c26e62

rh-atomic-bot · 2017-10-20T12:59:35Z

⌛ Testing commit 6c26e62 with merge 9166605...

This is a similar approach as coreos/rpm-ostree@12c34bb. One thing to note is when we parse the admin related functions, we still keep the old admin related flags, and added a new parameter to represent the command struct. This allows us to identify the caller of the function, making it easier for us to possibly deduplicate the subcommand handling in the future. A similar approach is done in rpm-ostree: coreos/rpm-ostree@83aeb01 This also makes it easier for us to change the prototype of the function. If we want to add something new in the future, we won't need to touch every prototype. Closes: #1267 Approved by: cgwalters

This is similar idea as coreos/rpm-ostree@5c0bf88, The duplicated description is now removed, and the description of the command is now displayed beneath the Usage. For example: ostree cat -h will output the following: "Usage: ostree cat [OPTION?] COMMIT PATH... Concatenate contents of files" Closes: #1267 Approved by: cgwalters

rh-atomic-bot · 2017-10-20T13:20:24Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 9166605 to master...

peterbaouoft · 2017-10-20T13:43:30Z

Thanks for the review :)

peterbaouoft changed the title ~~ostree: Describe subcommands in help output~~ WIP:ostree: Describe subcommands in help output Oct 13, 2017

jlebon added the WIP label Oct 13, 2017

jlebon reviewed Oct 13, 2017

View reviewed changes

peterbaouoft added 3 commits October 17, 2017 18:05

fixup! ostree: Describe subcommands in help output

1ad2095

peterbaouoft force-pushed the describe_subcommands_in_help branch from a25ada4 to 808a446 Compare October 17, 2017 18:12

peterbaouoft added 2 commits October 17, 2017 20:42

fixup! ostree: move flags into command struct, pass down through buil…

8615c61

…tins

peterbaouoft changed the title ~~WIP:ostree: Describe subcommands in help output~~ ostree: Describe subcommands in help output Oct 19, 2017

cgwalters reviewed Oct 19, 2017

View reviewed changes

cgwalters removed the WIP label Oct 19, 2017

fixup! ostree: provide command description in a better place

6c26e62

cgwalters added the homu/approved label Oct 20, 2017

rh-atomic-bot closed this in 3c360a7 Oct 20, 2017

peterbaouoft deleted the describe_subcommands_in_help branch October 20, 2017 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ostree: Describe subcommands in help output #1267

ostree: Describe subcommands in help output #1267

peterbaouoft commented Oct 13, 2017

peterbaouoft commented Oct 13, 2017

jlebon commented Oct 13, 2017

jlebon Oct 13, 2017

jlebon Oct 13, 2017

jlebon Oct 13, 2017

peterbaouoft Oct 13, 2017

jlebon Oct 13, 2017

peterbaouoft commented Oct 17, 2017

cgwalters Oct 19, 2017

cgwalters Oct 19, 2017

cgwalters Oct 19, 2017

cgwalters commented Oct 19, 2017

peterbaouoft commented Oct 19, 2017

peterbaouoft commented Oct 19, 2017 •

edited

Loading

cgwalters commented Oct 20, 2017

rh-atomic-bot commented Oct 20, 2017

rh-atomic-bot commented Oct 20, 2017

peterbaouoft commented Oct 20, 2017

ostree: Describe subcommands in help output #1267

ostree: Describe subcommands in help output #1267

Conversation

peterbaouoft commented Oct 13, 2017

peterbaouoft commented Oct 13, 2017

jlebon commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Oct 19, 2017

peterbaouoft commented Oct 19, 2017

peterbaouoft commented Oct 19, 2017 • edited Loading

cgwalters commented Oct 20, 2017

rh-atomic-bot commented Oct 20, 2017

rh-atomic-bot commented Oct 20, 2017

peterbaouoft commented Oct 20, 2017

peterbaouoft commented Oct 19, 2017 •

edited

Loading