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

Update init experience #3933

Merged
merged 4 commits into from
Sep 27, 2022
Merged

Update init experience #3933

merged 4 commits into from
Sep 27, 2022

Conversation

AaronCrawfis
Copy link
Contributor

@AaronCrawfis AaronCrawfis commented Sep 27, 2022

Description

  • Fix double semicolon
  • Simplify kubeconfig selection
  • Fix cursor position

Issue reference

Partially addresses

Partially fixes: radius-project/core-team#946

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Adds necessary unit tests for change
  • Adds necessary E2E tests for change
  • Unit tests passing
  • Extended the documentation / Created issue for it

@AaronCrawfis AaronCrawfis requested a review from a team as a code owner September 27, 2022 17:49
@@ -38,8 +38,7 @@ func SelectEnvironmentName(cmd *cobra.Command, defaultVal string, interactive bo
return "", err
}
if interactive && envStr == "" {
promptMsg := fmt.Sprintf("Enter an environment name [%s]:", defaultVal)
envStr, err = prompter.RunPrompt(prompt.TextPromptWithDefault(promptMsg, defaultVal, prompt.ResourceName))
envStr, err = prompter.RunPrompt(prompt.TextPromptWithDefault("Enter an environment name", defaultVal, prompt.ResourceName))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because promptui already has a default experience, we don't need to manually build the prompt message with the default

if strings.ToLower(confirmDefaultContext) == "y" {
return currentContext, nil
}
defaultValue := fmt.Sprintf("%s (current)", currentContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplify selection process by placing current at the top and appending (current), and just using a selection prompt

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it. This is one of the ideas we discussed a few weeks ago, glad to see it come back.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests might fail though because it is one skipped terminal interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, getting this error:

=== RUN   Test_Validate/Valid_Init_Command
    shared.go:73: assertion failed: expected error to contain "cluster unreachable", got "KubeContext not specified": Error should be: kubernetes cluster unreachable, got: KubeContext not specified

Copy link
Contributor

Choose a reason for hiding this comment

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

should be good now

@rynowak
Copy link
Contributor

rynowak commented Sep 27, 2022

/cc @bjoginapally

@AaronCrawfis
Copy link
Contributor Author

Double prompts are caused by manifoldco/promptui#129, which has an open PR. Just pinged the maintainers to review.

@@ -76,6 +74,7 @@ func SelectNamespace(cmd *cobra.Command, defaultVal string, interactive bool, pr
}
return nil
},
AllowEdit: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This places the cursor at the end of the line instead of the beginning

Copy link
Contributor

Choose a reason for hiding this comment

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

In the other case, the user can either press enter or start typing. In this case the user will have to clear the default first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, in the old case you could start typing and it would clear the current entry. In this new case you have to hit backspace to clear it out manually and then type the new name.

Given that the cursor blocks out the first character, I think this change is preferrable.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an alternative we can switch to https://github.com/charmbracelet/bubbletea, which has better support for default values (greyed out text)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree cursor blocking is not great. LGTM.

@AaronCrawfis AaronCrawfis merged commit b0cb333 into main Sep 27, 2022
@AaronCrawfis AaronCrawfis deleted the aacrawfi/init-update branch September 27, 2022 20:15
mishrapratikshya pushed a commit that referenced this pull request Sep 27, 2022
* Fix double semicolon and cursor location

* Update kubeconfig selection logic

* Auto formatting

* Correct integration tests

Co-authored-by: Bharath Joginapally <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants