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

feat: add kubectl auto-completion #108

Closed
wants to merge 1 commit into from

Conversation

AObuchow
Copy link
Contributor

@AObuchow AObuchow commented Jul 25, 2023

Resolves the last part of eclipse-che/che#21735 by adding bash autocompletion for kubectl.

To test, use the following image quay.io/aobuchow/universal-developer-image:kubectl-completion, i.e.:

docker run -ti --rm \
       quay.io/aobuchow/universal-developer-image:kubectl-completion \
       bash

Then, with a shell inside the container, type in kubectl - and hit TAB twice to see autocomplete results.
The output should resemble the following:

projects $ kubectl -
--add-dir-header            --client-key                --log-dir=                  --namespace=                --skip-log-headers          --v=
--alsologtostderr           --client-key=               --log-file                  --password                  --stderrthreshold           --vmodule
--as                        --cluster                   --log-file-max-size         --password=                 --stderrthreshold=          --vmodule=
--as-group                  --cluster=                  --log-file-max-size=        --profile                   --tls-server-name           --warnings-as-errors
--as-group=                 --context                   --log-file=                 --profile-output            --tls-server-name=          -n
--as=                       --context=                  --log-flush-frequency       --profile-output=           --token                     -s
--cache-dir                 --insecure-skip-tls-verify  --log-flush-frequency=      --profile=                  --token=                    -v
--cache-dir=                --kubeconfig                --loglevel                  --request-timeout           --user                      
--certificate-authority     --kubeconfig=               --loglevel=                 --request-timeout=          --user=                     
--certificate-authority=    --log-backtrace-at          --logtostderr               --server                    --username                  
--client-certificate        --log-backtrace-at=         --match-server-version      --server=                   --username=                 
--client-certificate=       --log-dir                   --namespace                 --skip-headers              --v 

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

A part for a minor suggestion it worked for me

Comment on lines 216 to 222
echo '[ -f ~/.kubectl_aliases ] && source ~/.kubectl_aliases' >> /home/user/.bashrc
EOF

# kubectl completion
RUN kubectl completion bash > /usr/share/bash-completion/completions/kubectl \
&& echo "source /usr/share/bash-completion/completions/kubectl" >> /home/user/.bashrc

Copy link
Contributor

Choose a reason for hiding this comment

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

I would move everything related to kubectl in the dedicated heredoc

Suggested change
echo '[ -f ~/.kubectl_aliases ] && source ~/.kubectl_aliases' >> /home/user/.bashrc
EOF
# kubectl completion
RUN kubectl completion bash > /usr/share/bash-completion/completions/kubectl \
&& echo "source /usr/share/bash-completion/completions/kubectl" >> /home/user/.bashrc
kubectl completion bash > /usr/share/bash-completion/completions/kubectl
echo '[ -f ~/.kubectl_aliases ] && source ~/.kubectl_aliases' >> /home/user/.bashrc
echo "source /usr/share/bash-completion/completions/kubectl" >> /home/user/.bashrc
EOF

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Looking at the .bashrc in the container, I see a warning that sdkman only works if the relevant lines are at the end of .bashrc, but we've injected a bunch of source lines after that block -- is the sdkman warning still true? cc: @l0rd

An easier way to do completions (but one that would require reworking all completions in the dockerfile) would be something like what WTO does:

  • In .bashrc: source /etc/profile.d/bash_completion.sh [1]
  • In dockerfile: put completions in $COMPDIR: [2]

@amisevsk
Copy link
Contributor

Further detail on the sdkman concern: this is the current .bashrc in the image:

#THIS MUST BE AT THE END OF THE FILE FOR SDKMAN TO WORK!!!
export SDKMAN_DIR="$HOME/.sdkman"
[[ -s "$HOME/.sdkman/bin/sdkman-init.sh" ]] && source "$HOME/.sdkman/bin/sdkman-init.sh"

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion
source /usr/share/bash-completion/completions/git
source /usr/share/bash-completion/completions/oc
[ -f ~/.kubectl_aliases ] && source ~/.kubectl_aliases
export PATH="${KREW_ROOT:-$HOME/.krew}/bin:$PATH"

@AObuchow
Copy link
Contributor Author

Looking at the .bashrc in the container, I see a warning that sdkman only works if the relevant lines are at the end of .bashrc, but we've injected a bunch of source lines after that block -- is the sdkman warning still true? cc: @l0rd

An easier way to do completions (but one that would require reworking all completions in the dockerfile) would be something like what WTO does:

Thanks for pointing this out & suggesting an alternative approach @amisevsk. I'm going to take a bit longer to work on this PR to try and see if I can rework all the completions in the dockerfile.

@l0rd
Copy link
Contributor

l0rd commented Jul 26, 2023

@amisevsk I don't know what's the side effect of the SDKMAN being initialized earlier in the .bashrc file, but I agree that we should fix it and your solution is elegant so let's do that.

@AObuchow if you want to do that work in a separate PR is ok too and we can merge this one in the meantime.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

+1 to merge with @l0rd's suggestion from review, and then optionally rework completions as a separate PR

@openshift-ci openshift-ci bot added the lgtm label Jul 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 28, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, AObuchow, l0rd

The full list of commands accepted by this bot can be found 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

@AObuchow
Copy link
Contributor Author

AObuchow commented Aug 2, 2023

I think this PR can be closed as it's made redundant by https://github.com/devfile/developer-images/pull/107/files#diff-9f21bef4669d474b6ddef409447be53ae91c94cb50b6df9345149a0e89fb5391R229 @l0rd

@l0rd
Copy link
Contributor

l0rd commented Aug 2, 2023

I think this PR can be closed as it's made redundant by #107 (files) @l0rd

Sorry, that was a mistake. I wanted to test your PR and I rebased mine on top of yours and then I forget about it and pushed your changes too 🤷 . So yes I think we can close this one.

@AObuchow
Copy link
Contributor Author

AObuchow commented Aug 2, 2023

@l0rd no worries 😎 sounds good, closing.

@AObuchow AObuchow closed this Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants