Skip to content

oc __custom_func clobbers kubectl __custom_func #371

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

Closed
commonquail opened this issue Apr 5, 2020 · 4 comments
Closed

oc __custom_func clobbers kubectl __custom_func #371

commonquail opened this issue Apr 5, 2020 · 4 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@commonquail
Copy link

commonquail commented Apr 5, 2020

kubectls Bash completion defines a function creatively named __custom_func, which serves as a sort of entry point to certain kubectl commands (config, create, get, etc.). Not all, and it's not entirely clear whether there is a pattern to the covered commands.

I've verified this in kubectl v1.11.0+d4cacc0 and v1.14.0.

oc' s Bash completion also defines a function named __custom_func, and it serves exactly the same purpose for oc as kubectl's __custom_func serves for kubectl. Because "o" comes after "k", oc's __custom_func clobbers kubectl's. While we can largely use oc as a proxy for kubectl, kubectl's completion is considerably more refined than oc's completion of kubectl's operations; for instance, context completion.

I've verified this only in oc v3.11.0+0cbc58b but it is evidently still true. It looks to the naked eye as if it can be trivially renamed.

Of course, it doesn't really matter which function clobbers which because both kubectl and oc are being bad citizens. It surprises me that either tool has gotten away with an unnamespaced function for so long, especially considering that the other more than 400 total functions are all properly namespaced.

@commonquail
Copy link
Author

This is apparently spf13/cobra#694. It was resolved for kubectl v1.16 so I guess it's indirectly resolved for oc, too; insofar as nobody else uses Cobra, which, of course, everyone does.

Here is a patch that makes oc well-behaved:

From 46cfa93022b1a2e3c0776f922b77ae71df768d00 Mon Sep 17 00:00:00 2001
From: Mikkel Kjeldsen <[email protected]>
Date: Mon, 6 Apr 2020 13:40:40 +0200
Subject: [PATCH] Fix Cobra __custom_func collision

Cobra's programmable completion generator used to define a naked
__custom_func function that would collide amongst any Cobra user [1];
which is many, including both kubectl and oc. Cobra added support for a
scoped version of the function a while back [2], and kubectl has taken
advantage of that since v1.16 [3]. Although oc and kubectl no longer
collide with each other, oc can still collide with any other Cobra user,
so scope oc's __custom_func as well.

[1] spf13/cobra#694
[2] spf13/cobra#730
[3] See commit a4ee7e49b ("bump(*)", 2019-09-22)
---
 contrib/completions/bash/oc         | 2 +-
 contrib/completions/zsh/oc          | 2 +-
 pkg/cli/admin/oadm_bashcomp_func.go | 2 +-
 pkg/cli/cli_bashcomp_func.go        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/completions/bash/oc b/contrib/completions/bash/oc
index 3b96d05fe..eddea86a6 100644
--- a/contrib/completions/bash/oc
+++ b/contrib/completions/bash/oc
@@ -341,7 +341,7 @@ __oc_require_pod_and_container()
     return 0
 }
 
-__custom_func() {
+__oc_custom_func() {
     case ${last_command} in
  
         # first arg is the kind according to ValidArgs, second is resource name
diff --git a/contrib/completions/zsh/oc b/contrib/completions/zsh/oc
index 570ebb63b..b4a78eadf 100644
--- a/contrib/completions/zsh/oc
+++ b/contrib/completions/zsh/oc
@@ -483,7 +483,7 @@ __oc_require_pod_and_container()
     return 0
 }
 
-__custom_func() {
+__oc_custom_func() {
     case ${last_command} in
  
         # first arg is the kind according to ValidArgs, second is resource name
diff --git a/pkg/cli/admin/oadm_bashcomp_func.go b/pkg/cli/admin/oadm_bashcomp_func.go
index 8dd290863..28269a2b4 100644
--- a/pkg/cli/admin/oadm_bashcomp_func.go
+++ b/pkg/cli/admin/oadm_bashcomp_func.go
@@ -2,7 +2,7 @@ package admin
 
 const (
 	BashCompletionFunc = `
-__custom_func() {
+__oc_custom_func() {
     case ${last_command} in
         oadm_validate_master-config | oadm_validate_node-config)
             _filedir
diff --git a/pkg/cli/cli_bashcomp_func.go b/pkg/cli/cli_bashcomp_func.go
index 9dc320e6f..17846944f 100644
--- a/pkg/cli/cli_bashcomp_func.go
+++ b/pkg/cli/cli_bashcomp_func.go
@@ -91,7 +91,7 @@ __oc_require_pod_and_container()
     return 0
 }
 
-__custom_func() {
+__oc_custom_func() {
     case ${last_command} in
  
         # first arg is the kind according to ValidArgs, second is resource name
-- 
2.26.0

The patch applies cleanly on openshift-clients-4.5.0-202004042001 and I explicltly grant permission to use it in whole or in part.

I'm not going to submit a pull request because

  1. I can only build on openshift-clients-4.5.0-202003312031, and
  2. only with make SHELL:=/bin/bash GO_REQUIRED_MIN_VERSION:= verify-generated-completions, because of in-tree and upstream reliance on Makefile pipefail and the hard Go 1.13 dependency introduced in commit ae8530f ("Bump k8s to 1.18.0 and api/client-go/library", 2020-03-19), and
  3. if I build against that version I undo commit 2ad4ef9 ("Run hack/update-generated-completions.sh", 2020-03-24).

In the meantime, to anyone else stumbling upon this, personal completions subject to this Cobra error can be manually fixed with

for f in  ~/.local/share/bash-completion/completions/*
do
    if grep --fixed-strings '__custom_func()' "$f" >/dev/null
    then
        cobra_scope="$(basename -s .bash "$f" | tr -d _)" 
        sed -i 's|__custom_func|__'"$cobra_scope"'_custom_func|g' "$f"
    fi
done
unset cobra_scope

That will also clobber Cobra's compatibility fallback to __custom_func but the fix renders that fallback irrelevant.

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 5, 2020
@commonquail
Copy link
Author

/close

@openshift-ci-robot
Copy link

@commonquail: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

3 participants