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.

Sign up for GitHub

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

Jump to bottom

Improve alias generation performance #79

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Improve alias generation performance #79

merged 2 commits into from
Nov 22, 2023

Conversation

LFrobeen
Copy link
Contributor

This PR proposes the following changes:

  • Improve performance of the combinations function, allowing for generating aliases with more optional args in reasonable runtime,
  • Add checks and warnings if conflicts aliases are detected (helps identifying if newly added args would cause clashing aliases)
  • Allow w flag to be used in conjunction output format flags like oyaml, ojson and owide

The main motivation for the PR was addressing the performance of the combinations function which I've noticed to be prohibitively slow if more args are added. Many of the permutations generated by the combinations include incompatible flags which can be filtered out early to make the overall generation run faster.

Moreover, this PR adds a few more permutations of the w, ojson, oyaml and owide flags. Previously the incompatibility definitions on the w flag tried to prevent it being used with json, yaml and wide outputs. However, due to the way incompatibilities were evaluated (look-behind check) the script still emitted permutations where w was followed by ojson etc, but not the other way around. This PR makes the incompatibility check global (i.e. not only look-behind) and allows w to be used with output formats in any order.

@LFrobeen LFrobeen marked this pull request as ready for review November 19, 2023 12:33
@ahmetb
Copy link
Owner

ahmetb commented Nov 19, 2023

This is superb, I've been hoping to optimize this but I rarely regenerate aliases so I never bothered.

@ahmetb
Copy link
Owner

ahmetb commented Nov 21, 2023

Allow watch flag to be used with json, yaml and wide flags f168a28

do you mind if we drop this commit? I think it increases alias count, hence the load time 35% for no good use case really.

@LFrobeen
Copy link
Contributor Author

Allow watch flag to be used with json, yaml and wide flags f168a28

do you mind if we drop this commit? I think it increases alias count, hence the load time 35% for no good use case really.

Sure, no problem. One thing to note though is that this will remove any combinations of w with ojson, oyaml etc, whereas the original implementation allowed w followed by ojson (e.g.

kubectl-aliases/.kubectl_aliases

Lines 335 to 352 in b2ee5db

alias kgwojson='kubectl get --watch -o=json'
alias ksysgwojson='kubectl --namespace=kube-system get --watch -o=json'
alias kgpowojson='kubectl get pods --watch -o=json'
alias ksysgpowojson='kubectl --namespace=kube-system get pods --watch -o=json'
alias kgdepwojson='kubectl get deployment --watch -o=json'
alias ksysgdepwojson='kubectl --namespace=kube-system get deployment --watch -o=json'
alias kgstswojson='kubectl get statefulset --watch -o=json'
alias ksysgstswojson='kubectl --namespace=kube-system get statefulset --watch -o=json'
alias kgsvcwojson='kubectl get service --watch -o=json'
alias ksysgsvcwojson='kubectl --namespace=kube-system get service --watch -o=json'
alias kgingwojson='kubectl get ingress --watch -o=json'
alias ksysgingwojson='kubectl --namespace=kube-system get ingress --watch -o=json'
alias kgcmwojson='kubectl get configmap --watch -o=json'
alias ksysgcmwojson='kubectl --namespace=kube-system get configmap --watch -o=json'
alias kgsecwojson='kubectl get secret --watch -o=json'
alias ksysgsecwojson='kubectl --namespace=kube-system get secret --watch -o=json'
alias kgnowojson='kubectl get nodes --watch -o=json'
alias kgnswojson='kubectl get namespaces --watch -o=json'
). Please let me know if this behaviour was intended. If not, I could probably modify my code so that it preserves the original semantics of the look-behind incompatibility check. What are your thoughts on this @ahmetb?

@ahmetb
Copy link
Owner

ahmetb commented Nov 21, 2023

No that wasn't intended I think.

@LFrobeen
Copy link
Contributor Author

I've removed the commit in question (f168a28). The new behaviour is that w won't be combined with any output formats in any order which reduces the total number of aliases by 459.

@ahmetb ahmetb merged commit ac5bfb0 into ahmetb:master Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LFrobeen @ahmetb