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

Fixes #5046, zsh completion #5072

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Fixes #5046, zsh completion #5072

merged 1 commit into from
Jan 29, 2019

Conversation

Sarke
Copy link
Contributor

@Sarke Sarke commented Dec 18, 2018

Signed-off-by: Peter Stalman sarkedev@gmail.com

What this PR does / why we need it:
Closes #5046

Special notes for your reviewer:
Small fix. Removes quotes in aliashash keys to work with ZSH.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Signed-off-by: Peter Stalman <sarkedev@gmail.com>
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 18, 2018
@kalinon
Copy link

kalinon commented Dec 27, 2018

Unfortunately this did not solve my ZSH bash error:

./bin/helm ins_helm_root_command:12: bad math expression: operand expected at `"del"'                                                             130 ↵
_helm_root_command:12: bad math expression: operand expected at `"del"'
_helm_root_command:12: bad math expression: operand expected at `"del"'
./bin/helm

This seems to be an issue with the OSX version of sed:

$ helm completion zsh | sed -e 's/aliashash\["\(\w\+\)"\]/aliashash[\1]/g' | grep aliashash  
	-e 's/aliashash\["\(\w\+\)"\]/aliashash[\1]/g' \
        # aliashash variable is an associative array which is only supported in bash > 3.
            words[c]=${aliashash[${words[c]}]}
        aliashash["ls"]="list"
        aliashash["up"]="update"
        aliashash["rm"]="remove"
        aliashash["up"]="update"
        aliashash["del"]="delete"
        aliashash["dep"]="dependency"
        aliashash["dependencies"]="dependency"
        aliashash["hist"]="history"
        aliashash["ls"]="list"
    declare -A aliashash 2>/dev/null || :
$ sed --help
sed: illegal option -- -
usage: sed script [-Ealn] [-i extension] [file ...]
       sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...]

However GNU sed seems to work:

$ gsed -E 's/aliashash\["(\w+)"\]/aliashash[\1]/g' ./comp.sh | grep aliashash
	-e 's/aliashash\["\(\w\+\)"\]/aliashash[\1]/g' \
        # aliashash variable is an associative array which is only supported in bash > 3.
            words[c]=${aliashash[${words[c]}]}
        aliashash[ls]="list"
        aliashash[up]="update"
        aliashash[rm]="remove"
        aliashash[up]="update"
        aliashash[del]="delete"
        aliashash[dep]="dependency"
        aliashash[dependencies]="dependency"
        aliashash[hist]="history"
        aliashash[ls]="list"
    declare -A aliashash 2>/dev/null || :
$ gsed --version
gsed (GNU sed) 4.7

@kalinon
Copy link

kalinon commented Dec 27, 2018

This sed seems to work with BSD sed:

helm completion zsh | sed -e "s/aliashash\\[\"\\(${LWORD}.*${RWORD}\\)\"\\]/aliashash[\\1]/g" | grep aliashash
	-e 's/aliashash\["\([[:<:]].*[[:>:]]\)"\]/aliashash[\1]/g' \
        # aliashash variable is an associative array which is only supported in bash > 3.
            words[c]=${aliashash[${words[c]}]}
        aliashash[ls]="list"
        aliashash[up]="update"
        aliashash[rm]="remove"
        aliashash[up]="update"
        aliashash[del]="delete"
        aliashash[dep]="dependency"
        aliashash[dependencies]="dependency"
        aliashash[hist]="history"
        aliashash[ls]="list"
    declare -A aliashash 2>/dev/null || :

Changing the line to use the word boundaries should make this compatible for others:

"s/aliashash\\[\"\\(${LWORD}.*${RWORD}\\)\"\\]/aliashash[\\1]/g"

@philoserf
Copy link

I believe the answer is to revert
#4851

@Sarke
Copy link
Contributor Author

Sarke commented Dec 27, 2018

@philoserf, that will only break the ZSH completion. So yes, you wouldn't get the error, but you would also not get any completion.

The issue is the introduction of the shorter aliases for bash, which are not correctly converted to to ZSH.

@bacongobbler bacongobbler merged commit 4c1edcf into helm:master Jan 29, 2019
@bacongobbler bacongobbler added this to the 2.13.0 milestone Jan 29, 2019
adamreese pushed a commit to adamreese/helm that referenced this pull request Mar 7, 2019
Signed-off-by: Peter Stalman <sarkedev@gmail.com>
(cherry picked from commit 4c1edcf)
adamreese pushed a commit to adamreese/helm that referenced this pull request Mar 7, 2019
Signed-off-by: Bartel Sielski <bsielski@nalys-group.com>
(cherry picked from commit 4c1edcf)
adamreese pushed a commit to adamreese/helm that referenced this pull request Mar 7, 2019
Signed-off-by: Peter Stalman <sarkedev@gmail.com>
(cherry picked from commit 4c1edcf)
adamreese pushed a commit to adamreese/helm that referenced this pull request Mar 7, 2019
Signed-off-by: Peter Stalman <sarkedev@gmail.com>
(cherry picked from commit 4c1edcf)
eraac pushed a commit to eraac/helm that referenced this pull request Mar 21, 2019
Signed-off-by: Peter Stalman <sarkedev@gmail.com>
Signed-off-by: Kevin Labesse <kevin@labesse.me>
marckhouzam added a commit to VilledeMontreal/helm that referenced this pull request May 20, 2019
PR helm#5072 followed by helm#5406 tweaked the handling of the associative
array 'aliashash' when in zsh.  However, upon further investigation
the root of the problem was the 'aliashash' was not being declared
properly as it was declared within a method and therefore not
accessible to the rest of the completion script.

The previous commit of this PR makes the necessary change to properly
declare 'aliashash' which makes the previous tweak unecessary.  This
commit removes the tweak.

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@marckhouzam
Copy link
Member

Turns out that the real root of this bug was that the 'aliashash' associative array was not being declared properly. PR #5680 fixes it and removes this fix, which is no longer needed.

@marckhouzam
Copy link
Member

And the saga continues. It seems that removing the quotes is necessary but for a completely different reason. See 48f0e31 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm completion fails in zsh (prezto) with "bad math expression: operand expected"
6 participants