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.

Sign up for GitHub

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

Jump to bottom

Add tab completion script for zsh and fix bash completion for ABS services #90

Merged
merged 8 commits into from
Sep 22, 2020
Merged

Add tab completion script for zsh and fix bash completion for ABS services #90

merged 8 commits into from
Sep 22, 2020

Conversation

scotje
Copy link
Contributor

@scotje scotje commented Aug 21, 2020

Status

Ready for Merge

Description

Adds a new --hostnameonly option to floaty list to serve as more reliable plumbing for the completion scripts, then updates the bash completion script to use that option and implements a new zsh completion script with equivalent functionality to the bash script.

Reviewers

@puppetlabs/dio
@highb
@briancain

@scotje scotje requested review from briancain, highb and a team as code owners August 21, 2020 18:50
briancain
briancain suggested changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Just some feedback on the util function for printing. Haven't tested the zsh completion or anything.

lib/vmfloaty/utils.rb Show resolved Hide resolved
@scotje scotje requested a review from briancain August 24, 2020 23:22
briancain
briancain approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Looks good to me from a code perspective :) 👍

@scotje
Copy link
Contributor Author

scotje commented Aug 25, 2020

@puppetlabs/dio if you all have write access would you mind reviewing/merging when you have a chance?

@genebean
Copy link
Contributor

genebean commented Sep 3, 2020 •
edited
Loading

Overall this looks good to me @scotje but I do have one minor request: please update https://github.com/puppetlabs/vmfloaty#tab-completion to include your new zsh bit.

Oh, and really sorry I am just now getting to reviewing this! You are always welcome to ping in our slack channel for things like this too.

highb
highb approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@highb highb left a comment

Choose a reason for hiding this comment

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

The --hostnameonly flag is a nice general addition. Perhaps a fish shell completion script is coming soon? 😆

@scotje
Fix print_to_stderr option for Utils.pretty_print_hosts
40072e9 
Previously this option had no effect on the output. This commit makes
the option effective and also refactors the tests for this method to
cover this case.
@scotje
Add --hostnameonly option to floaty list --active
b31f44f 
Adds an option to the `floaty list` subcommand so that, when listing
active hosts, floaty will output just the hostnames, without any
additional information or formatting. Hostnames will be separated by a
newline.

This functionality is primarily intended for consuming by tooling (such
as the tab completion scripts).
@scotje
Add tab completion script for zsh
57e7542 
This commit adds a new tab completion script for zsh. It also
fixes the completion script for bash to work with ABS backends.
@scotje
(FIXUP) Collect hostnames in array before outputting
dee735e
@scotje
Add README for zsh tab completion
e269d71
@scotje
WIP: fixup specs after rebase
8546c1c
@scotje
Copy link
Contributor Author

scotje commented Sep 22, 2020

I rebased this and fixed the tests up as best I could but ran out of time to completely figure out how the default output format of ABS hosts had changed and what I needed to mock out to make those tests pass. If someone has time to fix the tests and wants to re-push this to a new PR, I'm fine with that. Otherwise I can take another look tomorrow.

@genebean
Copy link
Contributor

@sbeaulie is this maybe something you have some insight into? Also, does anything here need adjusting as a result of #99 that I just merged?

Fix spec tests for ABS pretty print
45f0ef0 
ABS now returns the ABS job_id on the first line, then every
vmpooler hostname indented. It queries them from vmpooler to
get the metadata like lifetime etc
@sbeaulie
Copy link
Contributor

yes looking into fixing the tests now

Fix pretty print when multiple backend services
2b56448 
are returned by ABS. Added test with vmpooler and nspooler result
@sbeaulie
Copy link
Contributor

sbeaulie commented Sep 22, 2020 •
edited
Loading

I also found a bug and corrected it so that hybrid requests to ABS (vmpooler, nspooler) can pretty print the hostnames properly.

Your VMs on abs-prod.comp:
- [JobID:1600785162078] <allocated>
  - rosy-foreboding.comp (debian-8-x86_64, 0.36/12 hours, user: samuel)
  - sol10-11.comp (solaris-10-sparc)

@genebean genebean merged commit bc1ea2e into puppetlabs:master Sep 22, 2020
@scotje scotje deleted the add_zsh_completions_new branch September 22, 2020 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@highb highb highb approved these changes

@briancain briancain briancain approved these changes

@genebean genebean genebean approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@scotje @genebean @sbeaulie @highb @briancain