getistio fetch: add name flag in fetch #26

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

Merged

ZhiHanZ merged 9 commits into tetratelabs:main from ZhiHanZ:fetch-name

Mar 2, 2021

ZhiHanZ commented Mar 2, 2021

resolve #19


          add name flag in fetch

e2cb611

ZhiHanZ requested a review from a team

March 2, 2021 06:52


          fix UT

c84990e

mathetake reviewed

View reviewed changes

Member

mathetake left a comment

shall we add NAME column in getistio list command for convenience?

mathetake reviewed

View reviewed changes

src/istioctl/istioctl.go Outdated

+              	if len(name) != 0 {
+              		d, err := api.IstioDistributionFromString(name)
+              		if err != nil {
+              			return nil, fmt.Errorf("cannot parse given name to %s istio distribution", name)

Member

mathetake Mar 2, 2021 •

edited

Loading

Suggested change

      
            			return nil, fmt.Errorf("cannot parse given name to %s istio distribution", name)
          
            			return nil, fmt.Errorf("cannot parse given name %s to istio distribution", name)

Member

mathetake Mar 2, 2021

Could you do the same fix for https://github.com/tetratelabs/getistio/blob/ddc6cb8c3305eae74732fa7500fe5e64ed9bc59d/cmd/switch.go#L91 ?


          convert flag

mathetake requested changes

View reviewed changes

Member

mathetake left a comment •

edited

Loading

shall we put istioctl.processFetchParams in cmd/fetch.go and make istioct.Fetch accepting *api.IstioDistribution to be better aligned with switch cmd?

zhihanz added 2 commits

March 2, 2021 19:24


          fix UT

b5a4a47


          refactor

Signed-off-by: zhihanz <[email protected]>

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

Author

ZhiHanZ commented Mar 2, 2021

shall we add NAME column in getistio list command for convenience?
Created an issue on it


          minor changes

d9b16c3

mathetake requested changes

View reviewed changes

src/istioctl/istioctl.go Outdated Show resolved Hide resolved

src/istioctl/istioctl.go Outdated Show resolved Hide resolved


          refactor

ec2339b

mathetake reviewed

View reviewed changes

src/istioctl/istioctl_test.go Show resolved Hide resolved

mathetake reviewed

View reviewed changes

src/istioctl/istioctl_test.go Show resolved Hide resolved

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

mathetake reviewed

View reviewed changes

cmd/fetch.go Outdated Show resolved Hide resolved

zhihanz added 2 commits

March 2, 2021 20:14

nit

11c2cc2


          add old tests

mathetake approved these changes

View reviewed changes

Member

mathetake left a comment

LGTM

ZhiHanZ merged commit cc22fa9 into tetratelabs:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet