Re: [libvirt-jenkins-ci PATCH 2/5] ansible: Introduce the 'manage' tool

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2017-10-17 at 15:21 +0200, Pavel Hrdina wrote:
> > +do_list() {
> > +    INVENTORY=$(grep "^inventory\\s*=" ansible.cfg 2>/dev/null | sed "s/^.*=\\s*//g")
> > +    INVENTORY=${INVENTORY:-/etc/ansible/hosts}
> 
> I don't think that there is a need to include system-wide inventory
> since we have our own inventory and we don't use the system-wide
> inventory at all.

Good point. I turned the absence of the local inventory file into
an error, and...

> > +    grep -v '^\[' "$INVENTORY" | sort -u

... improved the regular expression so that empty lines and comments
will be ignored in addition to group names.

> > +case "$1" in
> > +    list)           do_list ;;
> > +    prepare|update) do_prepare "$2" ;;
> > +    *help)          do_help ;;
> > +    *)              die "Usage: $PROGRAM_NAME ACTION [OPTIONS]" ;;
> 
> How about grouping *help) and *) together?  I was pretending to be a
> basic user and I've run this script without any option at all and this
> was the only output.  So it would be nice to print the help itself
> or mention that there is some "--help" option.

I went one step further and dropped the command-specific error
message in favor of displaying the help text. One less string, and
it's actually more informative :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux