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 completions for Manifest commands #1910

Conversation

TomSweeneyRedHat
Copy link
Member

This adds the completions for the manifest commands
found in #1902. This should not be merged until that
is merged, or perhaps this should be cherry-picked
into it.

Signed-off-by: TomSweeneyRedHat [email protected]

@rhatdan
Copy link
Member

rhatdan commented Oct 13, 2019

LGTM

@TomSweeneyRedHat TomSweeneyRedHat changed the title DO NOT MERGE - Add completions for Manifest commands Add completions for Manifest commands Oct 29, 2019
@TomSweeneyRedHat
Copy link
Member Author

TomSweeneyRedHat commented Oct 29, 2019

I've removed the "Do Not Merge" label as #1902 has now merged. This should be gtg now and still has happy green test buttons.
@vrothberg @QiWang19 @nalind PTAL

@rhatdan
Copy link
Member

rhatdan commented Oct 29, 2019

Don't you need to add manifest as a command to _buildah()?
Then we need a _buildah_manifest() section to get all of these, I believe?

@TomSweeneyRedHat TomSweeneyRedHat force-pushed the dev/tsweeney/manifestbash branch from 0a31e6e to bc6f561 Compare October 29, 2019 17:05
@TomSweeneyRedHat
Copy link
Member Author

@rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Oct 31, 2019

@QiWang19
Copy link
Contributor

LGTM

-*)
COMPREPLY=($(compgen -W "$boolean_options $options_with_args" -- "$cur"))
;;
esac
Copy link
Member

Choose a reason for hiding this comment

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

This single-item case (?) is repeated four or five times, could it be made into a function or simplified some other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it could be improved, but we've it all over the place in this file and I'm just staying between the lines....

create
inspect
push
remove
Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace - I'm curious why the commit hooks didn't catch this?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, thx.

@edsantiago
Copy link
Member

This all gives me the sense of wandering into cargo-cult territory: the NOP-y declaration of all_options, the unnecessary splitting of boolean_options and options_with_args. I'm sure the intentions were good in the early days of this completion, but right now it's just copy-paste incantations without meaning. I feel like we should maybe drop the pretense, just declare a single options list and keep it clean. (Not on existing code, just on new additions). That would be easier to read, and IMO less misleading.

(Not intended as a slight on your work -- this completion stuff is HARD).

This adds the completions for the manifest commands
found in containers#1902.  This should not be merged until that
is merged, or perhaps this should be cherry-picked
into it.

Signed-off-by: TomSweeneyRedHat <[email protected]>
@TomSweeneyRedHat TomSweeneyRedHat force-pushed the dev/tsweeney/manifestbash branch from bc6f561 to 9843320 Compare October 31, 2019 23:36
@rhatdan
Copy link
Member

rhatdan commented Nov 1, 2019

Perhaps we get an intern next summer to concentrate on improving command completions, Make them work under @edsantiago :^)

@rhatdan
Copy link
Member

rhatdan commented Nov 1, 2019

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit 9843320 has been approved by rhatdan

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit 9843320 with merge 1967973...

@rh-atomic-bot
Copy link
Collaborator

💔 Test failed - status-travis

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr, status-travis
Approved by: rhatdan
Pushing 1967973 to master...

caiges pushed a commit to caiges/buildah that referenced this pull request Nov 12, 2019
This adds the completions for the manifest commands
found in containers#1902.  This should not be merged until that
is merged, or perhaps this should be cherry-picked
into it.

Signed-off-by: TomSweeneyRedHat <[email protected]>

Closes: containers#1910
Approved by: rhatdan
@TomSweeneyRedHat TomSweeneyRedHat deleted the dev/tsweeney/manifestbash branch January 8, 2020 18:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
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.

6 participants