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

Bash completion #35451

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Conversation

RonaldBarnes
Copy link

@RonaldBarnes RonaldBarnes commented Nov 27, 2022

Summary

Add bash completion for occ

TODO

  • Support more OS's bash_completion.d directory locations
  • Add any missed web server user names (i.e. nginx)

Checklist

  • Code is properly formatted N/A for bash scripts?
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required (requires confirmation of pending acceptance, then this PR's final state will be documented and a PR to nextcloud/documentation will be made)
  • Backports requested where applicable (ex: critical bugfixes) (Is this a good idea? Wouldn't hurt and easy to integrate.)

@RonaldBarnes
Copy link
Author

RonaldBarnes commented Nov 28, 2022

An example of the script in action, showing user interaction required before any action taken:

# . ./bash-add-alias.sh
Web server user name: "www-data".
Alias for occ command not found for user "root".
Run "alias occ='sudo --user www-data php /var/www/nextcloud/occ'" (y/N)? Y
alias occ='sudo --user www-data php /var/www/nextcloud/occ'
Add alias to /root/.bash_aliases? (y/N) N
Add alias to /home/ron/.bash_aliases? (y/N) N
Run bash completion script complete.occ?  (Y/n) Y
Running /var/www/nextcloud/complete.occ ... success.
Add complete.occ to /etc/bash_completion.d? (y/N) N
DONE.

@RonaldBarnes
Copy link
Author

RonaldBarnes commented Nov 28, 2022

Works from any folder, even when occ is not in the current directory:

root@(dell):/tmp/nextcloud-occ-bash-completion# occ
activity:        encryption:      memories:        text:
app:             federation:      notification:    theming:
background:      files:           onlyoffice:      trashbin:
background-job:  group:           preview:         twofactorauth:
broadcast:       help             recognize:       update:
check            integrity:       security:        upgrade
circles:         l10n:            serverinfo:      user:
config:          list             sharing:         versions:
dav:             log:             status           workflows:
db:              maintenance:     tag:
deck:            maps:            talk:
root@(dell):/tmp/nextcloud-occ-bash-completion# occ

Reduce number of choices by typing ta:

root@(dell):/tmp/nextcloud-occ-bash-completion# occ ta
tag:   talk:
root@(dell):/tmp/nextcloud-occ-bash-completion# occ ta

Increase choices by typing l, which completes to talk:, then provides all talk: options:

root@(dell):/tmp/nextcloud-occ-bash-completion# occ talk:
talk:active-calls         talk:room:delete          talk:stun:add
talk:command:add          talk:room:demote          talk:stun:delete
talk:command:add-samples  talk:room:promote         talk:stun:list
talk:command:delete       talk:room:remove          talk:turn:add
talk:command:list         talk:room:update          talk:turn:delete
talk:command:update       talk:signaling:add        talk:turn:list
talk:room:add             talk:signaling:delete     talk:user:remove
talk:room:create          talk:signaling:list
root@(dell):/tmp/nextcloud-occ-bash-completion# occ talk:

And so on:

root@(dell):/tmp/nextcloud-occ-bash-completion# occ talk:command:
talk:command:add          talk:command:delete       talk:command:update
talk:command:add-samples  talk:command:list
root@(dell):/tmp/nextcloud-occ-bash-completion# occ talk:command:list

@RonaldBarnes
Copy link
Author

RonaldBarnes commented Nov 28, 2022

Works with options beginning with - too, if user types in occ -[TAB]:

root@(dell):/tmp/nextcloud-occ-bash-completion# occ -
--ansi            --no-ansi         --quiet           --version
-h                --no-interaction  -v                -vv
--help            --no-warnings     -V                -vvv
-n                -q                --verbose
root@(dell):/tmp/nextcloud-occ-bash-completion# occ -

@szaimen szaimen added this to the Nextcloud 26 milestone Nov 28, 2022
@szaimen szaimen added enhancement 3. to review Waiting for reviews labels Nov 28, 2022
@szaimen szaimen requested a review from a team November 28, 2022 10:01
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on current behavior:

# 1
# Comment: Is having a triling ':' expected ? Not sure if we should remove it or not.
# Having it is weird, but it suggests that occ is expecting something after the ':'.
# How are other software handling that ?
$ occ<tab>
activity:        dav:             integrity:       preview:         text:            versions:
app:             db:              l10n:            security:        theming:         workflows:
background:      encryption:      list             serverinfo:      trashbin:        
background-job:  federation:      log:             sharing:         twofactorauth:   
broadcast:       files:           maintenance:     status           update:          
check            group:           migrations:      support:         upgrade          
config:          help             notification:    tag:             user:  

# 2
$ occ files:<tab>
files:cleanup                    files:repair-tree                files:scan-app-data
files:recommendations:recommend  files:scan                       files:transfer-ownership

# 3
# Comment: I don't think have the global list of command makes sense here.
# I would maybe expect completion on the options.
$ occ files:scan <tab>
activity:        dav:             integrity:       preview:         text:            versions:
app:             db:              l10n:            security:        theming:         workflows:
background:      encryption:      list             serverinfo:      trashbin:        
background-job:  federation:      log:             sharing:         twofactorauth:   
broadcast:       files:           maintenance:     status           update:          
check            group:           migrations:      support:         upgrade          
config:          help             notification:    tag:             user:      

complete.occ Outdated Show resolved Hide resolved
bash-add-alias.sh Outdated Show resolved Hide resolved
bash-add-alias.sh Outdated Show resolved Hide resolved
bash-add-alias.sh Outdated Show resolved Hide resolved
bash-add-alias.sh Outdated Show resolved Hide resolved
bash-add-alias.sh Outdated Show resolved Hide resolved
complete.occ Outdated Show resolved Hide resolved
@artonge artonge added the pending documentation This pull request needs an associated documentation update label Nov 28, 2022
complete.occ Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

@RonaldBarnes
Copy link
Author

It's odd that there are a couple comments here that I can't seem to reply to.

Is this normal; am I missing something?

@RonaldBarnes
Copy link
Author

@juliushaertl

Isn't this something that symfony already provides us with as documented in https://github.com/nextcloud/documentation/blob/eae31ad02fa83e218d693f7a79eded3d127e7406/admin_manual/configuration_server/occ_command.rst#enabling-autocompletion ?

According to stecman/symfony-console-completion#71, it doesn't seem work for everyone.

The author stated:

to be honest this seems like a permissions / design issue with Nextcloud, not the completion system. A few suggestions for working around this:

Configure file/dir permissions so that www-data and the owning user can both write to those directories:

Really don't like the idea of changing file/dir permissions within the NC folder to get bash comlpletion working! I imagine a lot of chmod 0777 commands being run.

Also - the method at the link you provided doesn't work for me - I copied and pasted the "BASH (any version)" instructions:

/var/www/nextcloud# eval $(/var/www/html/nextcloud/occ _completion --generate-hook)
-bash: /var/www/html/nextcloud/occ: No such file or directory

Instructions point to different NC directory, adjusting for local instance:

/var/www/nextcloud# ls -l occ
-rw-r--r-- 1 www-data www-data 283 2022-11-08 03:04 occ
root@(dell):/var/www/nextcloud# eval $(/var/www/nextcloud/occ _completion --generate-hook)
-bash: /var/www/nextcloud/occ: Permission denied
/var/www/nextcloud#

User root has permission denied. Trying again as user www-data:

/var/www/nextcloud# sudo -u www-data eval $(/var/www/nextcloud/occ _completion --generate-hook)
-bash: /var/www/nextcloud/occ: Permission denied
sudo: eval: command not found
/var/www/nextcloud#

Re-created my own occ alias, then:

/var/www/nextcloud# occ status
- installed: true
- version: 24.0.7.1

That completion method is not very discoverable - I know multiple NC admins (not full-time admins, but regulars) that had no idea this Symfony method existed.

This version is discoverable and functions like every other bash completion.

@RonaldBarnes
Copy link
Author

Hi again @artonge

I couldn't reply directly to your comment, so... "Quote Reply" is here:

Comments on current behavior:

# 1
# Comment: Is having a trailing ':' expected ?

Yes, it's an indicator that further subcommands are expected.

Not sure if we should remove it or not.

I strongly recommend keeping it; it's quite informative.

Having it is weird, but it suggests that occ is expecting something after the ':'.

Precisely.

How are other software handling that ?

I am not aware of any other program that expects colon-delimited input like occ.

#3
# Comment: I don't think have the global list of command makes sense here.
# I would maybe expect completion on the options.

I like the idea, but I see some problems with implementation.

  • Running the command to capture and parse the output looking for options would run the command, and capture the output for processing.
    If no further options expected by the command, then the command has run and output is hidden from user.
  • Some commands have quite long run-times, so things will appear to have hanged from user's perspective
  • Idempotence might be imperilled - critical output could be captured and re-running command might return different results.

I do think I could prevent displaying the global list of commands if there is already a word with a colon in it.

Also, if there were a list of commands that expect files as final arguments, I could make it so that those commands, followed by [tab], present a list of files and folders from the current directory.

I'm unaware of any such commands that operate on files though.

$ occ files:scan <tab>
activity:        dav:             integrity:       preview:         text:            versions:
app:             db:              l10n:            security:        theming:         workflows:
background:      encryption:      list             serverinfo:      trashbin:        
background-job:  federation:      log:             sharing:         twofactorauth:   
broadcast:       files:           maintenance:     status           update:          
check            group:           migrations:      support:         upgrade          
config:          help             notification:    tag:             user:      

RonaldBarnes added a commit to RonaldBarnes/NextCloud-server that referenced this pull request Dec 1, 2022
…mplete' built-in handles de-duplication and sortation.

Signed-off-by: Ronald Barnes <ron@ronaldbarnes.ca>
@RonaldBarnes
Copy link
Author

Completions are now customized to the expectations of the command being run, i.e. a user_id:

occ files:scan [tab][tab]
admin  A   J1   J2


occ files:scan J[tab][tab]
J1  J2

A user that's been chosen / selected will be filtered from further options:

occ files:scan J1 [tab][tab]
admin  A   J2

If -p or --path is the previous word, expect a file system entry:

root@(dell):/var/www/nextcloud# occ files:scan --path
3rdparty/          core/              occ                status.php
apps/              cron.php           ocm-provider/      test.sh
...

Note: in the case of files:scan --path, it must be run from ../nextcloud/data for file completion to give a valid option (a user's directory), or the argument to --path must be prefixed by /: this is not enforced by the completion script.

@RonaldBarnes
Copy link
Author

Completions are now customized to the expectations of the command being run, i.e. an app:

occ talk:command:list
accessibility              memories
activity                   nextcloud_announcements
admin_audit                notes
...

Lengthy list of all apps has pagination handled as any other bash completion (i.e. ls -l /usr/bin/[tab][tab]).

occ talk:command:list user_[tab][tab]
user_ldap    user_status

@RonaldBarnes
Copy link
Author

Completions are now customized to the expectations of the command being run, i.e. an app followed by a lang (language):

occ l[tab][tab]
l10n:  list   log:


occ l1[tab]
occ l18n:[tab]
occ l10n:createjs[tab][tab]
accessibility              memories
activity                   nextcloud_announcements
admin_audit                notes
apporder                   notifications
breezedark                 oauth2
...

Having chosen an app, now l18n expects a language file:

occ l10n:createjs user_ldap [tab][tab]
3rdparty/          core/              occ                status.php
apps/              cron.php           ocm-provider/      test.sh
AUTHORS            data/              ocs/               themes/
...

bash-add-alias.sh Outdated Show resolved Hide resolved
@MichaIng
Copy link
Member

MichaIng commented Dec 5, 2022

Since aliases are usually added via ~/.bashrc or /etc/bash.bashrc, and hence the script runs on every interactive bash session, isn't it a bit verbose and also huge for the trivial task of a single alias that could be a single line like:

alias occ='sudo -u www-data php /var/www/nextcloud/occ'

What I suggest is using this script as one time setup script, which adds the resulting single alias line to ~/.bashrc (or optionally any other defined file). This also renders the whole trap stuff and variable cleanup obsolete (since the script doesn't need to be sourced, but can be executed), which currently make half of the script.

But otherwise, especially the bash completion itself: Great stuff 👍🚀!!

@RonaldBarnes
Copy link
Author

Since aliases are usually added via ~/.bashrc or /etc/bash.bashrc, and hence the script runs on every interactive bash session, isn't it a bit verbose and also huge for the trivial task of a single alias that could be a single line like:

alias occ='sudo -u www-data php /var/www/nextcloud/occ'

The bash-add-aliases.sh is a one-time script. Assuming the user chooses to have the alias it generates added to some file for persistence.

I like the ~/.bash_aliases location much better than inside ~/.bashrc as the alias file is often sourced that way (if it exists), and I have a lot of aliases I like to keep separate.

Had to check a bunch of default installs and was surprised this wasn't the standard setup. It's been so long since I created ~/.bash_aliases that I don't recall doing it. And, it's auto-pushed to all my systems, so I expected that to be normal.

What I suggest is using this script as one time setup script, which adds the resulting single alias line to ~/.bashrc (or optionally any other defined file). This also renders the whole trap stuff and variable cleanup obsolete (since the script doesn't need to be sourced, but can be executed), which currently make half of the script.

How about, it is a one-time setup script which builds the single alias line, puts it in ~/.bash_aliases files, if they exist, else puts it into /etc/bash.bashrc if user chooses, and keeps the traps so if a user interrupts the procedure, the environment still gets cleaned up?

But otherwise, especially the bash completion itself: Great stuff +1rocket!!

Thank you! Really appreciate that.

@MichaIng
Copy link
Member

MichaIng commented Dec 6, 2022

I like the ~/.bash_aliases location much better than inside ~/.bashrc as the alias file is often sourced that way (if it exists), and I have a lot of aliases I like to keep separate.

Ah you are right, that is even better.

How about, it is a one-time setup script which builds the single alias line, puts it in ~/.bash_aliases files, if they exist, else puts it into /etc/bash.bashrc if user chooses, and keeps the traps so if a user interrupts the procedure, the environment still gets cleaned up?

~/.bash_aliases could be also created, if it's missing. Writing to /etc/bash.bashrc of course requires root permissions, but a nice optional feature, to enable the alias for all users.

Btw, bash-completion also reads from ~/.local/share/bash-completion/completions, so the completion itself can be also installed on user-level, if the script is not executed as root, or it is not wanted for all users.

So overall, also to allow running the script in shared hostings, it would be great if the script used the user-level directories for both, alias and completion, and the system-wide dirs only when executed as root (and probably still optionally).


Ah right, it does things already as one-time-script. Then I wonder whether it's worth it being sourced and setting up everything for the current shell at all? Is it worth the hassle (and still not ruled out 100% risk that the current shell is unintentionally manipulated by the variables, functions and traps) to avoid the need for a exec bash (or logout/login) after it finished?

@RonaldBarnes
Copy link
Author

~/.bash_aliases could be also created, if it's missing. Writing to /etc/bash.bashrc of course requires root permissions, but a nice optional feature, to enable the alias for all users.

I thought about creating the file, but I checked a Debian Bullseye (11) and it does not source an aliases file. Same with Fedora 37. Ubuntu 22.04 both defines aliases in ~/.bashrc and sources ~/.bash_aliases if it exists; both the best and worst choices.

Btw, bash-completion also reads from ~/.local/share/bash-completion/completions, so the completion itself can be also installed on user-level, if the script is not executed as root, or it is not wanted for all users.

That's good to know, I was not aware.

So overall, also to allow running the script in shared hostings, it would be great if the script used the user-level directories for both, alias and completion, and the system-wide dirs only when executed as root (and probably still optionally).

Initially, I tried to keep changes mostly in the invoking user's environment.

I will consider offering to put complete.occ into ~/.local/share/bash-completion/completions/ and how best to give the user the option between there and the global /etc/bash_completion.d.

However, since running occ requires the user to have some sudo privileges, I don't think having root access as a requirement is critical? Or, in a shared hosting situation, is config/config.php owned by the user, not the web server (or does it have 0664 permissions))?

Maybe if user has sudoers privileges to run some things as www-data but writing to /etc/bash_completion.d/ fails, then I could offer to write to ~/.local/share/bash-completion/completions?

Ah right, it does things already as one-time-script. Then I wonder whether it's worth it being sourced and setting up everything for the current shell at all?

I think so. When gaining shell access first time, run the script, and alias and completions are ready to help with the task at hand.

Is it worth the hassle (and still not ruled out 100% risk that the current shell is unintentionally manipulated by the variables, functions and traps) to avoid the need for a exec bash (or logout/login) after it finished?

I'm not clear on this part.

I may be overlooking something, but the only bit of remaining code that I have concern about is the global var COMP_WORDBREAKS having the colon stripped out.

That may affect something somehow somewhere, but I haven't encountered any other completion that depends on it being there and I have not been able to get the completions to work if I unset the changes I made to that variable.

(The change affects how the complete built-in works, which exists outside the _occ function entirely, unfortunately. Initially expected it worked with the compgen built-in, but no such luck.

@MichaIng
Copy link
Member

MichaIng commented Dec 6, 2022

Ah I see, ~/.bash_aliases is not sourced by bash directly, but only indirectly via ~/.bashrc if distro default or admin added it. Then I wouldn't bother with ~/.bash_aliases at all, as it's basically not guaranteed to work, but add it only to ~/.bashrc, which is always loaded by bash directly.

I don't think having root access as a requirement is critical?

It really depends on the environment. In shared hostings, the provided login user may be the webserver user at the same time, in which case sudo is not required at all. In other cases, the login user might have sudo permissions, but probably not for root. But maybe too many checks aren't great either, at least not for now. We can react to specific environments once users face issues with the script in their particular environment, and otherwise assume/require a login user with sudo permissions, or root itself, for now.

Choice for whether to add globally or for current user resp. SUDO_USER only probably via command-line option and/or interactive read (as done for other inputs already)?

I'm not clear on this part.

I mean, large parts of the scripts deal with traps and unsetting variables, to not mess with the current shell (the script is sourced into) which is all not required when the script is executed instead of sourced. The only downside is that one cannot run occ immediately after executing the script, but needs to refresh the shell session to have the alias and bash completion loaded. This however is pretty common. A hint that one needs to logout/login/refresh/open a new shell session, easiest via exec bash to replace the current bash session with a new one.

I may be overlooking something, but the only bit of remaining code that I have concern about is the global var COMP_WORDBREAKS having the colon stripped out.

I don't mean the completion script itself (which of course needs to be sourced), but bash-add-alias.sh, which I think complicates things when being sourced instead of executed.

EDIT: Of course these parts are then obsolete/do not work anymore:

Run "alias occ='sudo --user www-data php /var/www/nextcloud/occ'" (y/N)? Y
...
Run bash completion script complete.occ?  (Y/n) Y

But I do not think there are cases where one wants to load the alias + completion only once, and not install it for future logins.

@RonaldBarnes
Copy link
Author

Interesting issue: if complete.occ is copied to /etc/bash_completion.d/, it works.
If it's copied to ~/.local/share/bash-completion/completions/, it does not work. In that location, it's required to be named one of the following:

  • occ
  • occ.bash
  • _occ

I now copy** it to ~/.local/share/bash-completion/completions/occ but I really don't like the idea of having a different file name, in case someone goes looking for complete.occ on their system.

** Only copy it to ~/.local/share/bash-completion/completions/ if it fails to copy to /etc/bash_completion.d/occ, which is the most likely target I think, or /etc/bash_completion.d/ does not exist.

I'm wondering if it's best to just rename complete.occ to occ.bash now, so wherever it or its copies reside, they always have the same name?

@RonaldBarnes
Copy link
Author

Ah I see, ~/.bash_aliases is not sourced by bash directly, but only indirectly via ~/.bashrc if distro default or admin added it. Then I wouldn't bother with ~/.bash_aliases at all, as it's basically not guaranteed to work, but add it only to ~/.bashrc, which is always loaded by bash directly.

The work has been done; I really don't want to undo it.

Plus, ~/.bash_aliases is the better location for aliases even if it's inconsistently used.
Similar to ~/.bash_profile, ~/.bash_logout, and /.bash_history.

I should add an option to add the alias to the end of ~/.bashrc if ~/.bash_aliases is not found though.

I don't think having root access as a requirement is critical?

It really depends on the environment. In shared hostings, the provided login user may be the webserver user at the same time, in which case sudo is not required at all. In other cases, the login user might have sudo permissions, but probably not for root. But maybe too many checks aren't great either, at least not for now. We can react to specific environments once users face issues with the script in their particular environment, and otherwise assume/require a login user with sudo permissions, or root itself, for now.

I haven't had to deal with shared hosting for a long time, but I agree with your conclusion.

There'll be a lot of permutations of deployed environments that will reveal issues to work out when they appear.

Choice for whether to add globally or for current user resp. SUDO_USER only probably via command-line option and/or interactive read (as done for other inputs already)?

I'm not clear on this part.

I mean, large parts of the scripts deal with traps and unsetting variables, to not mess with the current shell (the script is sourced into) which is all not required when the script is executed instead of sourced. The only downside is that one cannot run occ immediately after executing the script, but needs to refresh the shell session to have the alias and bash completion loaded. This however is pretty common. A hint that one needs to logout/login/refresh/open a new shell session, easiest via exec bash to replace the current bash session with a new one.

I wouldn't have thought that an issue. User logs in to server, runs bash script, issues occ commands, logs out. Don't want to force them to re-start bash; I just don't see any benefit but a lot of friction.

But I do not think there are cases where one wants to load the alias + completion only once, and not install it for future logins.

That seems possible, and is supported.

@RonaldBarnes
Copy link
Author

Thinking about shared hosting scenarios, maybe instead of throwing error if config/config.php is not owned by the web server user, give a warning message then create the alias with the sudo --user= having a value of the owner of the config/config.php file?

@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 28, 2024
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@sorbaugh
Copy link
Contributor

Hello @RonaldBarnes , fyi I'm reopening this and would like to re-discuss this as the feature looks really nice and looks quite complete.

@sorbaugh sorbaugh reopened this Aug 16, 2024
@RonaldBarnes
Copy link
Author

fyi I'm reopening this and would like to re-discuss this as the feature looks really nice and looks quite complete.

Thank you, this is great news.

I find this tool extraordinarily handy on several Nextcloud instances running different versions - if someone else in the Nextcloud community can make use of it too, that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto tab completion for bash