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

[CI:DOCS] Fix fish completion issue if the command is prefixed with a space #9079

Merged
merged 1 commit into from Jan 25, 2021

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 24, 2021

Update the completion script like spf13/cobra#1249.

Fixes #8829

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2021
Update the completion script like spf13/cobra#1249.

[NO TESTS NEEDED]

Fixes containers#8829

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@Luap99 Luap99 changed the title Fix fish completion issue if the command is prefixed with a space [CI:DOCS] Fix fish completion issue if the command is prefixed with a space Jan 24, 2021
@edsantiago
Copy link
Collaborator

Well, this sure brings up an interesting monkey wrench!

As I interpret it, this PR is a manually-done commit, cherrypicking specific cobra fixes but not bringing in the new cobra itself. The obvious question then is, won't the automatically-generated completions override this change?

Ha ha, joke's on me. There are no automatically-generated completions. We're shipping old completion files with no updates.

Effective this week, this will change. I will be adding a CI test that will (1) implement a make completions target; (2) run it in CI; (3) fail if any completion file differs from what is checked in. I'll also fix completions/Readme.md. The goal is to prevent completion files from getting out of sync with podman or cobra.

@Luap99 I'm afraid that my proposed change will undo your PR. Since mine is just a proposal, not yet implemented or even agreed-upon by the team, I'm OK with this PR merging; but we need to start the conversation right now re: bringing in new cobra.

Thanks as always for raising great questions that improve our code base!

@Luap99
Copy link
Member Author

Luap99 commented Jan 24, 2021

Effective this week, this will change. I will be adding a CI test that will (1) implement a make completions target; (2) run it in CI; (3) fail if any completion file differs from what is checked in. I'll also fix completions/Readme.md. The goal is to prevent completion files from getting out of sync with podman or cobra.

Ed, I already had this done when we originally merged my completions PR #6442.

The problem is that upstream cobra does not have the nice features and bug fixes that I want. There are a lot of open completion PRs. #6442 was merged with my cobra fork. This was not sustainable as pointed out in #8528. So I reverted this #8534. I concluded that it is the best user experience to keep the new (unmerged) scripts.

I was really hoping that this issue would have been resolved by now, but none of the cobra PRs have been merged.

I just got the reminder about #8829 and said whatever, I will fix this issue myself. I do not like to maintain this but I rather give a better user experience than waiting for upstream to merge the fixes.

@Luap99
Copy link
Member Author

Luap99 commented Jan 24, 2021

To give some more context. I am talking about the following cobra PRs:

Except spf13/cobra#1258 because this one needs changes in the go code all of the other PRs are already included in our completions files in the completions directory. The podman completion command creates old scripts with several problems addressed in those PRs.

@edsantiago
Copy link
Collaborator

Point taken, thank you. I will hold off on my CI checks.

I can't think of any way to track this problem, other than for a human to watch for a new upstream release, then vendor it in, then alert me to go ahead with the new checks. Can you think of a better way?

@Luap99
Copy link
Member Author

Luap99 commented Jan 25, 2021

I can't think of any way to track this problem, other than for a human to watch for a new upstream release, then vendor it in, then alert me to go ahead with the new checks. Can you think of a better way?

That's what I am doing. I follow upstream closely.

@TomSweeneyRedHat
Copy link
Member

LGTM
but I'm not at all a fish expert.
Happy green test buttons.

@mheon
Copy link
Member

mheon commented Jan 25, 2021

/lgtm

I wonder if we should bring more attention to this upstream - would be nice to finally get those PRs merged

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2021
@openshift-merge-robot openshift-merge-robot merged commit f4e8572 into containers:master Jan 25, 2021
@Luap99 Luap99 deleted the fish-completion branch January 25, 2021 17:32
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fish completion not working as expected
6 participants