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

target history feature #162

Closed
wants to merge 12 commits into from
Closed

target history feature #162

wants to merge 12 commits into from

Conversation

tedteng
Copy link
Contributor

@tedteng tedteng commented Jun 15, 2022 •
edited
Loading

What this PR does / why we need it:
target history feature and fuzzy targeting

image

Which issue(s) this PR fixes:
Fixes #133
Fixes #135

Special notes for your reviewer:
the Fuzzy targeting required fzf installed
the target history feature only records target history, which means target view, and target unset do not count in the implement
PowerShell is not supported yet.
use gh as alias name call gardenctl target history feature

Release note:

targeting history feature and Fuzzy targeting feature, use `gh` as alias name call `gardenctl target history` feature

@tedteng tedteng requested a review from a team as a code owner June 15, 2022 05:54
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jun 15, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 15, 2022
@tedteng
Copy link
Contributor Author

tedteng commented Jun 15, 2022 •
edited
Loading

This PR is Ready for review
I don't have Win machine for Powershell alias code check. might need to create a separate ticket for this part

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 17, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 17, 2022
This was referenced Jun 22, 2022
Open
Open
@tedteng
Copy link
Contributor Author

tedteng commented Jun 29, 2022

This PR is ready for review, @gardener/gardenctl-v2-maintainers

@petersutter
Copy link
Member

Hi Ted,

I don't have Win machine for Powershell alias code check

-> brew install pwsh on your Mac

I checked how the binary / history feature behaved and it reminds me of your very first implementation proposal in gardenctl (v1). You only store the command itself but loose the complete context. We already discussed this excessively in the past and you had improved it in the v1 implementation so that the context is not lost. That's why I'm a bit surprised why this implementation is again a step backwards, compared to the v1 implementation.

@tedteng
Copy link
Contributor Author

tedteng commented Jun 29, 2022 •
edited
Loading

The context mean cluster info is it? let me know if I get wrong understanding. if yes, that feature is provided by github.com/manifoldco/promptui , as we drop promptui package this feature is also dismissed.
image

Didn't realize the cluster info still needed to display it. as eval $(gardenctl kubectl-env zsh) in zshrc + PS1 it also displays after targeted to identify the environment.
but I just check the native feature provided by fzf. it seems also available. I will take a look it. it might not 100% look like before but I will have a try

@petersutter
Copy link
Member

you do not only need the complete context for display purposes, but also to restore the target properly

@tedteng tedteng marked this pull request as draft June 30, 2022 07:48
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 5, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 5, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 5, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 5, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 6, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 6, 2022
@tedteng tedteng marked this pull request as ready for review July 6, 2022 01:21
@tedteng
Copy link
Contributor Author

tedteng commented Jul 6, 2022

This PR is ready for review, https://github.com/orgs/gardener/teams/gardenctl-v2-maintainers

@tedteng
Copy link
Contributor Author

tedteng commented Jul 18, 2022

Can we process this PR Thanks? @gardener/gardenctl-v2-maintainers

@petersutter
Copy link
Member

Hi @tedteng, somehow it does not write the history in my case
Screenshot 2022-07-19 at 11 47 19

When debugging it, it also did not run into PostRunE

gardenctl-v2/pkg/cmd/target/target.go

Lines 40 to 46 in 6be635b

PostRunE: func(cmd *cobra.Command, args []string) error {
s, err := HistoryParse(f, cmd)
if err != nil {
return err
}
return HistoryWrite(historyPath(f), s)
},

@tedteng
Copy link
Contributor Author

tedteng commented Jul 25, 2022

Hi @petersutter, I'm just back from the vacation. Did the issue still occur? I was try to on my local and one more new Mac not meet this error again.

image

version

image

@petersutter
Copy link
Member

@tedteng sorry for the late reply, yes the issue still occurs. @holgerkoser @grolu does it work for you?

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 9, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 9, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 16, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 16, 2023
@tedteng
Copy link
Contributor Author

tedteng commented Jan 16, 2023

PR is rebased feel free to process it thanks. @gardener/gardenctl-v2-maintainers

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Apr 27, 2023
@gardener-robot
Copy link

@tedteng You need rebase this pull request with latest master branch. Please check.

@tedteng tedteng mentioned this pull request May 19, 2023
Closed
@tedteng
Copy link
Contributor Author

tedteng commented Jun 9, 2023 •
edited
Loading

/close this PR. as it can simply create an alias after history to achieve the same effect. even in Win
eg

alias gg='eval $(history -n | grep -E "gardenctl target|g target" | grep -v history | uniq | fzf --tac  --no-sort --height 40% --layout reverse --info inline --border  --preview-window up,1,border-horizontal)'

@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petersutter petersutter Awaiting requested review from petersutter

@holgerkoser holgerkoser Awaiting requested review from holgerkoser

@grolu grolu Awaiting requested review from grolu

Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fuzzy targeting Target history
7 participants
@tedteng @petersutter @grolu @gardener-robot @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3