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

Default the target if there is exactly one #3430

Closed
wants to merge 3 commits into from

Conversation

@SvenDowideit
Copy link

SvenDowideit commented Mar 5, 2019

I'm guessing quite a lot of us have only one concourse target, so just using it would be nice

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 5, 2019

Interesting, I don't think anyone's proposed this before. This seems to be a safe compromise for simpler use cases (compared to having a configurable default or setting it in env). I'll think on it after 5.0 is out - thanks!

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 19, 2019

Well, haven't thought a whole lot about it to be honest but it does still seem reasonable.

@topherbullock @jama22 any thoughts on this?

Copy link
Member

vito left a comment

Thanks for submitting this! 👍

Left one note inline. Also would you mind adding tests for this?

flyTargets, err := LoadTargets()
if err != nil {
return TargetProps{}, err
}

if selectedTarget == "" {
if len(flyTargets.Targets) != 1 {
return TargetProps{}, ErrNoTargetSpecified

This comment has been minimized.

Copy link
@vito

vito Mar 19, 2019

Member

It might be worth giving this a distinct error message in the > 1 case. From a user's perspective "no target specified" would be confusing, since it was OK to not specify it until another target came around.

How about something like "no target specified and none available" for the 0 case, and "more than one target saved; target must be specified" for >1?

This comment has been minimized.

Copy link
@SvenDowideit

SvenDowideit Mar 20, 2019

Author

yup, sounds like a much more helpful UX

This comment has been minimized.

Copy link
@SvenDowideit
@vito vito added the needs-test label Mar 19, 2019
Signed-off-by: Dowideit, Sven (O&A, St. Lucia) <Sven.Dowideit@csiro.au>
Copy link
Member

vito left a comment

Thanks for adding tests - much better! Left a few more suggestions. 🙂

flyTargets, err := LoadTargets()
if err != nil {
return TargetProps{}, err
}

if selectedTarget == "" {
if len(flyTargets.Targets) == 0 {
return TargetProps{}, ErrNoTargetSpecifiedNoneAvailable

This comment has been minimized.

Copy link
@vito

vito Mar 25, 2019

Member

👍

return TargetProps{}, ErrNoTargetSpecifiedNoneAvailable
}
if len(flyTargets.Targets) > 1 {
return TargetProps{}, ErrNoTargetSpecifiedNeedToChoose

This comment has been minimized.

Copy link
@vito

vito Mar 25, 2019

Member

👍

fly/rc/targets.go Outdated Show resolved Hide resolved
fly/rc/targets_test.go Outdated Show resolved Hide resolved
@@ -49,12 +49,19 @@ func handleError(helpParser *flags.Parser, err error) {
fmt.Fprintln(ui.Stderr, "")
fmt.Fprintln(ui.Stderr, " "+ui.Embolden("fly -t %s login", commands.Fly.Target))
fmt.Fprintln(ui.Stderr, "")
} else if err == rc.ErrNoTargetSpecified {
fmt.Fprintln(ui.Stderr, "no target specified. specify the target with "+ui.Embolden("-t")+" or log in like so:")
} else if err == rc.ErrNoTargetSpecifiedNoneAvailable {

This comment has been minimized.

Copy link
@vito

vito Mar 25, 2019

Member

It might be worth adding tests for this behavior, since otherwise this isn't covered - the tests you added cover the conditions under which the error is returned, but not the messaging to the user.

It looks like some tests are failing due to the new message so maybe that suite could just be updated?

@vito vito removed the needs-test label Mar 25, 2019
vito and others added 2 commits Mar 25, 2019
Co-Authored-By: SvenDowideit <SvenDowideit@home.org.au>
Co-Authored-By: SvenDowideit <SvenDowideit@home.org.au>
@SvenDowideit

This comment has been minimized.

Copy link
Author

SvenDowideit commented Sep 18, 2019

i've decided not to use concourse for our work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.