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 bash and zsh completion scripts for openqa-cli #6063

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

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Nov 19, 2024

  • Allow static completion of the openqa-cli
  • Provide full cli options for each subcommand
  • Group options for each subcommand
  • Not help text in bash due to compgen function doesn't support descriptions

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

The only thing I don't like is the duplication. We would have the parameters now in four places:

There is a solution for that, but it is a bigger change, not sure if we want that here.

I wrote a program that can create pod and completion scripts from a YAML file.
The openqa-cli code could also read the YAML file, so we would only have the parameters in one place.
But it would be a new dependency, at least for developers.
(One cool thing is that it can generate completion with descriptions also for bash, with a trick)

I could at least post a proof of concept.

@perlpunk
Copy link
Contributor

I could at least post a proof of concept.

https://gist.github.com/perlpunk/b0d9fe14f3c877f28d8af3db6be28c70
Here is the demo with the bash completion including descriptions:
https://gist.github.com/perlpunk/b0d9fe14f3c877f28d8af3db6be28c70#file-demo-txt

@b10n1k
Copy link
Contributor Author

b10n1k commented Nov 20, 2024

The only thing I don't like is the duplication. We would have the parameters now in four places:

There is a solution for that, but it is a bigger change, not sure if we want that here.

I wrote a program that can create pod and completion scripts from a YAML file. The openqa-cli code could also read the YAML file, so we would only have the parameters in one place. But it would be a new dependency, at least for developers. (One cool thing is that it can generate completion with descriptions also for bash, with a trick)

I could at least post a proof of concept.

awesome and thanks for taking the time for the proof of concept.
in general, this was one idea for the hackweek, and learning stuff for me. I know that ideally it should be automatic in some way, but it was easier for me to provide something within the hackweek, and thats why I keep it as simple as possible. Not sure I am going to do the automation, as I believe is on another level for me and my plan is to look at another small project I am interested in. I hope we are not gonna be too restrict with this PR for now. It is so cool to have it and I have also additional ideas for it.
So manybe we could open some backlog tickets for the rest? wdyt?

@perlpunk

This comment was marked as resolved.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

It is not nice that we not only introduce one more place where we specify arguments but even two. Ideally we have a single source of truth. That would of course complicate things. Maybe we can make it so there's at least just one additional place where we specify arguments.

The code that does the argument parsing definitely needs to state in a comment that one also has to change these files when adding new parameters or changing help texts.

@b10n1k b10n1k force-pushed the autocompletion_cli branch from 23714aa to 298cf29 Compare November 21, 2024 07:03

This comment was marked as resolved.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Considering that @perlpunk already provided a proof of concept to reduce duplication with #6063 (comment) we should follow that approach

dist/rpm/openQA.spec Outdated Show resolved Hide resolved
dist/rpm/openQA.spec Outdated Show resolved Hide resolved
@b10n1k b10n1k force-pushed the autocompletion_cli branch 5 times, most recently from dc3d9dc to f75305f Compare November 22, 2024 08:11
dist/rpm/openQA.spec Outdated Show resolved Hide resolved
@b10n1k b10n1k force-pushed the autocompletion_cli branch 3 times, most recently from 6028003 to 95f0df9 Compare November 22, 2024 17:15
dist/rpm/openQA.spec Outdated Show resolved Hide resolved
dist/rpm/openQA.spec Outdated Show resolved Hide resolved
@b10n1k b10n1k force-pushed the autocompletion_cli branch 5 times, most recently from a37aad3 to ca87c8a Compare November 26, 2024 07:54
dist/rpm/openQA.spec Outdated Show resolved Hide resolved
@b10n1k b10n1k force-pushed the autocompletion_cli branch from ca87c8a to 73cf032 Compare November 26, 2024 13:08
completion/openqa-cli-completion.zsh Outdated Show resolved Hide resolved
'--help[Show help message]'
'--host[Target host, defaults to http://localhost]'
'--o3[Set target host to https://openqa.opensuse.org]'
'--osd[Set target host to http://openqa.suse.de]'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'--osd[Set target host to http://openqa.suse.de]'
'--osd[Set target host to https://openqa.suse.de]'

'--poll-interval[Specifies the poll interval in seconds]'
)
schedule_options=(
'--monitor[it until all jobs are done/cancelled and return non-zero exit code if at least on job has not passed/softfailed]'
Copy link
Member

Choose a reason for hiding this comment

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

Is the help message imply Monitor? Like this:

Suggested change
'--monitor[it until all jobs are done/cancelled and return non-zero exit code if at least on job has not passed/softfailed]'
'--monitor[Monitor it until all jobs are done/cancelled and return non-zero exit code if at least on job has not passed/softfailed]'

@b10n1k b10n1k force-pushed the autocompletion_cli branch 11 times, most recently from 60b29ee to 703167e Compare December 4, 2024 07:52
@okurz
Copy link
Member

okurz commented Dec 4, 2024

I tried out the sub-packages. From within podman run --rm -it registry.opensuse.org/opensuse/tumbleweed I did

zypper ar https://download.opensuse.org/repositories/devel:/openQA:/GitHub:/os-autoinst:/openQA:/PR-6063/openSUSE_Tumbleweed/ pr
zypper in openQA-client-bash-completion openQA-client-zsh-completion
rpm -ql openQA-client-zsh-completion-4.6.1733298771.703167ed-7471.1.x86_64
rpm -ql openQA-client-bash-completion-4.6.1733298771.703167ed-7471.1.x86_64

and found
/usr/share/zsh/site-functions/_openqa-cli
/usr/share/zsh/site-functions/_openqa-cli/openqa-cli-completion.zsh
/usr/share/bash-completion/completions/openqa-cli
/usr/share/bash-completion/completions/openqa-cli/openqa-cli-completion.bash

@perlpunk
Copy link
Contributor

perlpunk commented Dec 4, 2024

I wrote in #6063 (comment):

What we need in the end is this (and with those exact filenames):

/usr/share/zsh/site-functions/_openqa-cli
/usr/share/bash-completion/completions/openqa-cli

Those files need to be there, as files, not directories.

When you hit tab in bash, it looks into /usr/share/bash-completion/completions for a file with the same filename as the command. you can just have a look in that directory. You only see filenames there, right? No directories.
The same for zsh, just that the convention there is to prepend the file with an underscore.

Also I showed you the example for git-subrepo before. We looked at the output for rpm -ql together.

% rpm -ql git-subrepo-zsh-completion-0.4.6-16.4.noarch.rpm
/usr/share/zsh
/usr/share/zsh/site-functions
/usr/share/zsh/site-functions/_git-subrepo
% rpm -ql git-subrepo-bash-completion-0.4.6-16.4.noarch.rpm
/usr/share/bash-completion/completions/git-subrepo

Simply look at those files and think about what the files should look like for openqa-cli. Replace git-subrepo with openqa-cli.

@b10n1k b10n1k force-pushed the autocompletion_cli branch from 703167e to f5f50da Compare December 4, 2024 10:49
@b10n1k b10n1k force-pushed the autocompletion_cli branch 3 times, most recently from 693ecc7 to d35e888 Compare December 5, 2024 09:18
docs/UsersGuide.asciidoc Outdated Show resolved Hide resolved
* Allow static completion of the openqa-cli
* Provide full cli options for each subcommand
* Group options for each subcommand
* Not help text in bash due to `compgen` function doesn't support descriptions

Signed-off-by: Ioannis Bonatakis <[email protected]>
@b10n1k b10n1k force-pushed the autocompletion_cli branch 3 times, most recently from a99f139 to d52a34f Compare December 9, 2024 09:31
Provide openqa-cli bash completion for openqa-cli as a package when openQA is
install and bash is used.

Signed-off-by: Ioannis Bonatakis <[email protected]>
Signed-off-by: ybonatakis <[email protected]>
Signed-off-by: Ioannis Bonatakis <[email protected]>
Signed-off-by: ybonatakis <[email protected]>
@b10n1k b10n1k force-pushed the autocompletion_cli branch from d52a34f to b354e72 Compare December 9, 2024 13:10
@b10n1k b10n1k requested a review from okurz December 11, 2024 08:17
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Latest changes look good but please keep in mind that the major issue is still #6063 (review)

@@ -125,6 +125,17 @@ install-generic:
ln -s ../postgresql.service "$(DESTDIR)"/usr/lib/systemd/system/openqa-scheduler.service.requires/postgresql.service
install -d -m 755 "$(DESTDIR)"/usr/lib/systemd/system/openqa-websockets.service.requires
ln -s ../postgresql.service "$(DESTDIR)"/usr/lib/systemd/system/openqa-websockets.service.requires/postgresql.service
install -D -m 644 contrib/completions/openqa-cli-completion.bash "$(DESTDIR)"/usr/share//bash-completion/completions/openqa-cli
install -D -m 644 contrib/completions/openqa-cli-completion.zsh "$(DESTDIR)"/usr/share/zsh/site-functions/_openqa-cli
if [ "$(SHELL)" = "/bin/bash" ] || [ "$(SHELL)" = "/usr/bin/bash" ]; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add anything to a user's bashrc/zshrc by default when doing make install.
And if they have bash-completion installed, it shouldn't be necessary anyway. Same for zsh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants