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 basic ezjail-admin completion #4324

Merged
merged 1 commit into from Sep 6, 2017

Conversation

Projects
None yet
3 participants
@herrbischoff
Contributor

herrbischoff commented Aug 13, 2017

Description

Add basic completion for ezjail-admin utility on FreeBSD.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Aug 13, 2017

Member

I was going to suggest using string instead of awk, but I had a look at the zsh completions shipped with ezjail.

Those completions actually read the jail state files directly, so instead of screenscraping the output, you could do something like:

function __fish_complete_jails
   set -l jails /usr/local/etc/ezjail/*
   echo $jails
end

function __fish_complete_running_jails
    for j in (__fish_complete_jails)
        if test -f /var/run/jail_{$j}.id
             echo $j
        end
    end
end

etc.

Member

zanchey commented Aug 13, 2017

I was going to suggest using string instead of awk, but I had a look at the zsh completions shipped with ezjail.

Those completions actually read the jail state files directly, so instead of screenscraping the output, you could do something like:

function __fish_complete_jails
   set -l jails /usr/local/etc/ezjail/*
   echo $jails
end

function __fish_complete_running_jails
    for j in (__fish_complete_jails)
        if test -f /var/run/jail_{$j}.id
             echo $j
        end
    end
end

etc.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 13, 2017

Member
set -l jails /usr/local/etc/ezjail/*
echo $jails

These files need to be separated with a newline, not a space. Otherwise splitting won't be performed.

So either printf '%s\n' $jails, or for j in /usr/local/etc/ezjail/*; echo $j; end.

Member

faho commented Aug 13, 2017

set -l jails /usr/local/etc/ezjail/*
echo $jails

These files need to be separated with a newline, not a space. Otherwise splitting won't be performed.

So either printf '%s\n' $jails, or for j in /usr/local/etc/ezjail/*; echo $j; end.

@herrbischoff

This comment has been minimized.

Show comment
Hide comment
@herrbischoff

herrbischoff Aug 13, 2017

Contributor

These look like good suggestions. However, how about filtering for stopped jails? Also, isn't it true that a loop executes slightly slower than the screen scraping approach, especially when chaining two loops like with __fish_complete_running_jails?

Contributor

herrbischoff commented Aug 13, 2017

These look like good suggestions. However, how about filtering for stopped jails? Also, isn't it true that a loop executes slightly slower than the screen scraping approach, especially when chaining two loops like with __fish_complete_running_jails?

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 13, 2017

Member

Also, isn't it true that a loop executes slightly slower than the screen scraping approach, especially when chaining two loops like with __fish_complete_running_jails?

The screen scraping approach requires starting an external tool. The overhead of fork/exec is typically larger than a tiny loop. Also note that I also suggested an approach without a loop (the printf one).

I'd suggest testing if you are in doubt. My money is on the builtin-approach being faster, which is presumably also why the zsh completions do that.

Rule 1 of optimizing shell scripts: Use external tools as little as possible.

However, how about filtering for stopped jails?

I would assume there's a way without screen scraping there. Possibly by reading the files. Check the zsh completions.

Member

faho commented Aug 13, 2017

Also, isn't it true that a loop executes slightly slower than the screen scraping approach, especially when chaining two loops like with __fish_complete_running_jails?

The screen scraping approach requires starting an external tool. The overhead of fork/exec is typically larger than a tiny loop. Also note that I also suggested an approach without a loop (the printf one).

I'd suggest testing if you are in doubt. My money is on the builtin-approach being faster, which is presumably also why the zsh completions do that.

Rule 1 of optimizing shell scripts: Use external tools as little as possible.

However, how about filtering for stopped jails?

I would assume there's a way without screen scraping there. Possibly by reading the files. Check the zsh completions.

@herrbischoff

This comment has been minimized.

Show comment
Hide comment
@herrbischoff

herrbischoff Aug 13, 2017

Contributor

Additionally, @zanchey's suggested approach will output the full path of the jail instead of just the jail name. This leads to cluttered output. If this is what the Zsh completions do, I believe my approach is the cleaner one, as it also has the benefit of not requiring the jail configurations to be present in /usr/local/etc/ezjail, which could change in the future when full jail.conf compatibility is done.

All used tools (grep, awk) are included in FreeBSD.

I'm open to any kind of improvement. Also, it's just a suggestion and attempt to extend fish's usability. Maybe I should try to get it merged in ezjail and not here.

Contributor

herrbischoff commented Aug 13, 2017

Additionally, @zanchey's suggested approach will output the full path of the jail instead of just the jail name. This leads to cluttered output. If this is what the Zsh completions do, I believe my approach is the cleaner one, as it also has the benefit of not requiring the jail configurations to be present in /usr/local/etc/ezjail, which could change in the future when full jail.conf compatibility is done.

All used tools (grep, awk) are included in FreeBSD.

I'm open to any kind of improvement. Also, it's just a suggestion and attempt to extend fish's usability. Maybe I should try to get it merged in ezjail and not here.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 13, 2017

Member

which could change in the future when full jail.conf compatibility is done.

In that case, yeah, we want the command.

Member

faho commented Aug 13, 2017

which could change in the future when full jail.conf compatibility is done.

In that case, yeah, we want the command.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Aug 14, 2017

Member

Sorry, I left the stopped jails function out. I figured just inverting the test was a trivial addition.

This approach came from the completions for zsh shipped with ezjail, so it might be worth checking with them to see whether it is a stable approach.

Member

zanchey commented Aug 14, 2017

Sorry, I left the stopped jails function out. I figured just inverting the test was a trivial addition.

This approach came from the completions for zsh shipped with ezjail, so it might be worth checking with them to see whether it is a stable approach.

@faho faho added this to the fish-3.0 milestone Sep 6, 2017

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 6, 2017

Member

You know what? Let's just merge this for now. Any improvements can be made later.

Thanks @herrbischoff, and sorry for the delay!

Member

faho commented Sep 6, 2017

You know what? Let's just merge this for now. Any improvements can be made later.

Thanks @herrbischoff, and sorry for the delay!

@faho faho merged commit c0c33b3 into fish-shell:master Sep 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zanchey zanchey modified the milestones: fish 2.7.0, fish-3.0 Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment