Skip to content
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
Merged

Add basic ezjail-admin completion #4324

merged 1 commit into from Sep 6, 2017

Conversation

herrbischoff
Copy link
Contributor

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
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
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
@zanchey zanchey modified the milestones: fish 2.7.0, fish-3.0 Sep 6, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants