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

Nebari Typer CLI #1443

Merged
merged 47 commits into from
Oct 7, 2022
Merged

Nebari Typer CLI #1443

merged 47 commits into from
Oct 7, 2022

Conversation

asmijafar20
Copy link
Contributor

Fixes #1372

iameskild and others added 14 commits August 28, 2022 21:10
* Nebari cli: Add files _init.py and main.py

* run pre-commit

Co-authored-by: iameskild <[email protected]>
* Nebari typer cli commands

* add validate command

* Change validate command

* add precommit
* Nebari Render Command

* add render_template

* Add dry-run
* Nebari deploy and destroy commands

* Add typer confirm to destroy command

* Run pre-commit

Co-authored-by: eskild <[email protected]>
Co-authored-by: iameskild <[email protected]>
* Nebari deploy and destroy commands

* Add typer confirm to destroy command

* Add deploy arguments and change repository default value

* Add rendre design

* Run pre-commit

Co-authored-by: eskild <[email protected]>
Co-authored-by: iameskild <[email protected]>
@iameskild
Copy link
Member

iameskild commented Sep 21, 2022

@asmijafar20 based on today's feedback from the team, we need to make the following updates:

  • Undo dotenv work, replace with os.environ["ENV"] = "VAR" for now
  • Combine guided-init and init
  • Flesh out the questions and add more links to our docs
  • Combine CLI commands into sections

@iameskild iameskild added type: enhancement 💅🏼 New feature or request area: user experience 👩🏻‍💻 impact: high 🟥 This issue affects most of the nebari users or is a critical issue status: in review 👀 This PR is currently being reviewed by the team area: nebari-cli labels Sep 22, 2022
iameskild and others added 7 commits September 22, 2022 15:12
* Remove Usage: dotenv [OPTIONS] COMMAND [ARGS]
* Remove pathlib
* Add , ,  and  commands.

* Add pre-commit

* Add pre-commit

* Add pre-commit

* Add rich print

* Change do_keycloak function

* Create two keycloak subcommands

Co-authored-by: iameskild <[email protected]>
Co-authored-by: eskild <[email protected]>
@trallard
Copy link
Member

Hey folks reminder here that we also need instructions for folks in the team to use this new CLI + docs in the main docs page

@iameskild
Copy link
Member

Thanks a bunch @pavithraes for the thorough review.

  • After selecting a cloud provider in the "Where would you like to deploy your Nebari cluster?" question, we get prompts for "Please enter your GOOGLE_CREDENTIALS:", "Please enter your PROJECT_ID:", etc. -- Things typed/pasted here aren't visible to me, but seem to still get registered. Not sure if this is intentional, but I'd personally prefer to know if I typed/pasted the right thing and didn't make any typos here. :)

This is indeed intentional. Would it be better without? I might also change the verbiage to something like Paste your ...

  • Using CTRL+C to exit the wizard raises a very long error message. I think this can be slightly intimidating to new users, especially the AttributeError at the very end. Would it be possible to simplify this?

Good call! I made a few changes and this shouldn't happen any more 🤘

  • At the very end (when we display the validate and nebari init commands), maybe we can also include a sentence about next steps, something like:
You can now deploy your Nebari instance with:

$ nebari deploy -c nebari-config.yaml

For more information, run `nebari deploy --help` or check out the documentation: https://www.nebari.dev/how-tos/

Added :)

  • The white exclamation marks used across the new CLI weren't distinguishable when I was using light mode. We may want to use a different color here?

Definitely, good catch! Just swapped it out for ❗️

  • The comments (ones starting with the potted plant emoji) seem to be related to questions following it, but are visually closer to the previous questions. Maybe it's worth spacing them equidistant or closer to the following questions?

More space added.

@iameskild
Copy link
Member

@trallard thanks again for the thorough review :)

I believe I responded to all of your comments. I also opened an issue with a list of possible enhancements: #1478

iameskild and others added 2 commits October 3, 2022 19:29
* Add version Options to Nebari

* Add pre-commit

Co-authored-by: eskild <[email protected]>
@asmijafar20
Copy link
Contributor Author

I think the green colour for links is good. I just made it bold and it's visible in the light theme as well. I preferred green because it follows the Nebari logo colour theme which is really good. I have attached screenshots to decide on which looks better.
Screenshot from 2022-10-04 18-58-21

Screenshot from 2022-10-04 18-50-40

@asmijafar20
Copy link
Contributor Author

Just a mistake in the commit message it is spring_green to green

Comment on lines +56 to +57
if inputs.kubernetes_version == "latest":
inputs.kubernetes_version = None
Copy link
Member

Choose a reason for hiding this comment

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

righto - it is fine - what we need to make sure is to add a disclaimer / output saying that the latest available Kubernetes version will be installed as none was provided

rich.print(
(
"\n 🪴 Nebari comes with [green]Keycloak[/green], an open-source identity and access management tool. This is how users, groups and roles "
"are managed on the platform. To connect Keycloak with an identity provider, you can select one now.\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

yup that will work - might want to add a TODO: in this bit to remind us that it needs doing

Comment on lines 395 to 400
if git_provider == "github.com":
inputs.repository_auto_provision = questionary.confirm(
f"Would you like the following git repository to be automatically created: {inputs.repository}?",
default=False,
qmark=qmark,
).ask()
Copy link
Member

Choose a reason for hiding this comment

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

so if I say "no" would it still initialise the local git repo?

Comment on lines 395 to 400
if git_provider == "github.com":
inputs.repository_auto_provision = questionary.confirm(
f"Would you like the following git repository to be automatically created: {inputs.repository}?",
default=False,
qmark=qmark,
).ask()
Copy link
Member

Choose a reason for hiding this comment

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

To remove ambiguity I'd suggest something like "Would you like nebari to create a remote repository on f{GIT-provider}?" <- however that variable is

Comment on lines 444 to 448
inputs.namespace = questionary.text(
"What namespace would like to use?",
default=inputs.namespace,
qmark=qmark,
).ask()
Copy link
Member

Choose a reason for hiding this comment

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

much better

@trallard trallard added needs: changes 🧱 Review completed - some changes are needed before merging status: in review 👀 This PR is currently being reviewed by the team and removed status: in review 👀 This PR is currently being reviewed by the team labels Oct 4, 2022
@asmijafar20 asmijafar20 added needs: review 👀 This PR is complete and ready for reviewing and removed needs: changes 🧱 Review completed - some changes are needed before merging labels Oct 4, 2022
qhub/cli/_init.py Outdated Show resolved Hide resolved
@viniciusdc
Copy link
Contributor

LGTM, already!! I reviewed the code and couldn't find any flaws, once #1480 is integrated we can do the final review and merge (as the CI will be doing the end-to-end test for deploy and destroy)

@iameskild iameskild added this to the Release v0.4.5 milestone Oct 7, 2022
Copy link
Member

@trallard trallard left a comment

Choose a reason for hiding this comment

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

I think we can merge for now - there are plenty of improvements and suggestions in #1478

qhub/cli/_keycloak.py Show resolved Hide resolved
* Update CI to use new nebari cli entrypoint

* Add --disable-prompt to nebari init

* Update name of keycloak subcommands

* Reorder command

* Update command

* Minor update to test_e2e

* Update hostname used for test_deployment

* Another minor update to the tests_e2e

* Add --disable-prompt to nebari destroy

* use --disable-prompt in ci test

* Update .github/workflows/kubernetes_test.yaml

Co-authored-by: Vinicius D. Cerutti <[email protected]>

* Update kubernetes_test.yaml

* Update .github/workflows/kubernetes_test.yaml

Co-authored-by: Tania Allard <[email protected]>

* Update qhub/cli/main.py

Co-authored-by: Tania Allard <[email protected]>

* Fix yaml

* Fix yaml again

Co-authored-by: Vinicius D. Cerutti <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
@viniciusdc viniciusdc merged commit 440a75d into main Oct 7, 2022
@iameskild iameskild deleted the typer_cli branch October 7, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nebari-cli area: user experience 👩🏻‍💻 impact: high 🟥 This issue affects most of the nebari users or is a critical issue needs: review 👀 This PR is complete and ready for reviewing status: in review 👀 This PR is currently being reviewed by the team type: enhancement 💅🏼 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] - Improve QHub Setup experience
5 participants