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

Interactive namespace selection #1285

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

ddebarros
Copy link
Contributor

@ddebarros ddebarros commented Oct 18, 2022

  • Users a more interactive list for selecting a namespace.
  • Adds a new SpinningLoader component.
Screen.Recording.2022-10-18.at.11.40.03.AM.mov

…e-merge-main

Merge main into feature/serverless
…alocean#1232)

* Add support for triggers

* Add lastRun field to trigger list output

* Hide commands we won't be supporting in EA day 1

* Bump deployer version to pick up bug fix

* Fix error handling in services related to triggers

Many calls were not checking for errors.

* Switch to latest API

Change both the triggers command (native to doctl) and the deployer
version (which affects the semantics of deploy/undeploy).

* Pick up latest deployer (triggers bug fix)

* Remove support for prototype API and clean up code

* Fix unit tests

* Fix misleading comment

* Remove added complexity due to successive change

* Add filtering by function when listing triggers

* Fix omitted code in DeleteTrigger

* Guard triggers get/list with status check

Otherwise, the credentials read fails with a cryptic error instead of
an informative one when you are not connected to a namespace.
…-main

Merge latest `main` changes into `feature/serverless`
This completes the elimination of plugin usage in doctl sls fn and the
functions.go source file.
I believe this happened in merge conflict resolution during the recent
rebase.
Affects what happens when a failure occurs in the middle of deleting
functions and triggers together.
…on-list-no-plugin

Finish eliminating plugin usage in 'doctl sls functions' + testing improvements
…#1270)

* WIP for converting activations to direct OW flows

* Finish recoding 'activations get' in native doctl

Tests still to come

* Convert the support for sls actv result

Tests not converted yet

* Generate latet mocks

* Fix some comments

* Use more realistic timestampes

* Revise tests for new paths.  Still no output check

* Tests are now doing meaningful output comparison

Fixed some bugs found once tests were really effective
Re-writes activations list command subtree in Go
kamaln7
kamaln7 previously approved these changes Oct 19, 2022
Copy link
Contributor

@kamaln7 kamaln7 left a comment

Choose a reason for hiding this comment

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

nice one Davi!

commands/charm/spinner/spinner.go Outdated Show resolved Hide resolved
commands/namespaces.go Outdated Show resolved Hide resolved
Copy link
Contributor

@joshuaauerbachwatson joshuaauerbachwatson left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the code in detail but I want to raise a fundamental problem with the change. I tried it out, and it behaves nicely when I run it in a true terminal window. However, when I run it in an emacs shell (which is not a full-fledged terminal), or even in emacs term (which is more like a real terminal but with some emacs intermediation) the results are confusing.

In emacs shell, when selection from a prompt is required due to multiple namespace matches: things spew on the window and an informed selection is impossible. I'm not even going to try to show it here.

In emacs shall, when prompting is not required (an unambiguous namespace hint is provided), the experience is better but the "spinner" messages are annoying and keep the final message from being read easily:

ds connect lon
⣽  Loading namespaces ...⣻  Loading namespaces ...⢿  Loading namespaces ...⡿  Loading namespaces ...⣟  Loading namespaces ...⣯  Loading namespaces ...⣷  Loading namespaces ...⣾  Loading namespaces ...⣽  Loading namespaces ...⣻  Loading namespaces ...⢿  Loading namespaces ...Connected to functions namespace '<redacted>' on API host '<redacted>' (label=london)

In emacs term when a sufficient hint is provided, the result is satisfactory.

In emacs term when there is actual prompting, the prompt is impossible to use effectively because the cursor movements don't work as they would on a regular terminal (this might depend on emacs key bindings, not sure).

General points:

  • the spinner is IMO unnecessary and less is more there
  • the new prompt logic is nice to look at and might be fine in a full function terminal window but what was there before was good enough and by virtue of being minimal was more likely to work in more environments.

I'd be comfortable approving this iff the spinner was dropped and the improved selection experience were somehow guarded by a solid test for being in a real terminal. When the terminal is not "real", there should be a fallback to the simpler logic.

@ddebarros ddebarros changed the base branch from feature/serverless to main October 26, 2022 23:42
@ddebarros ddebarros dismissed kamaln7’s stale review October 26, 2022 23:42

The base branch was changed.

@ddebarros ddebarros changed the base branch from main to feature/serverless October 26, 2022 23:43
@ddebarros ddebarros changed the base branch from feature/serverless to main October 27, 2022 15:09
@ddebarros ddebarros marked this pull request as ready for review November 1, 2022 14:55
@ddebarros
Copy link
Contributor Author

I haven't reviewed the code in detail but I want to raise a fundamental problem with the change. I tried it out, and it behaves nicely when I run it in a true terminal window. However, when I run it in an emacs shell (which is not a full-fledged terminal), or even in emacs term (which is more like a real terminal but with some emacs intermediation) the results are confusing.

In emacs shell, when selection from a prompt is required due to multiple namespace matches: things spew on the window and an informed selection is impossible. I'm not even going to try to show it here.

In emacs shall, when prompting is not required (an unambiguous namespace hint is provided), the experience is better but the "spinner" messages are annoying and keep the final message from being read easily:

ds connect lon
⣽  Loading namespaces ...⣻  Loading namespaces ...⢿  Loading namespaces ...⡿  Loading namespaces ...⣟  Loading namespaces ...⣯  Loading namespaces ...⣷  Loading namespaces ...⣾  Loading namespaces ...⣽  Loading namespaces ...⣻  Loading namespaces ...⢿  Loading namespaces ...Connected to functions namespace '<redacted>' on API host '<redacted>' (label=london)

In emacs term when a sufficient hint is provided, the result is satisfactory.

In emacs term when there is actual prompting, the prompt is impossible to use effectively because the cursor movements don't work as they would on a regular terminal (this might depend on emacs key bindings, not sure).

General points:

  • the spinner is IMO unnecessary and less is more there
  • the new prompt logic is nice to look at and might be fine in a full function terminal window but what was there before was good enough and by virtue of being minimal was more likely to work in more environments.

I'd be comfortable approving this iff the spinner was dropped and the improved selection experience were somehow guarded by a solid test for being in a real terminal. When the terminal is not "real", there should be a fallback to the simpler logic.

I updated the Interactive to check if in a dumb terminal.

Copy link
Contributor

@joshuaauerbachwatson joshuaauerbachwatson left a comment

Choose a reason for hiding this comment

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

I tried out the latest version of this in emacs shell. Result:

Error: Namespace is required when running non-interactively

This is a regression in terms of UX. Before the change, I would have gotten the simple plain-text prompt. I am actually running interactively, albeit in a less than fully functional terminal. I was assuming that, when a deficient terminal is detected, the behavior would revert to limited plain text interactivity. This is what commands like man do.

I also tried this in emacs term (I don't generally use that but others undoubtedly do and it is often given as the solution when people find that things they want to use don't work in emacs shell). The behavior there is unchanged from before the latest change: the behavior is "interactive" but it is difficult and counterintuitive to read the prompts and make a selection due to the way the text is formatted. IMO the plain text solution would be better there as well.

I don't want to stand in the way of more interactivity in doctl (a good thing) and more use of state-of-the-art interaction paradigms, text coloring, etc. on real terminals (a good thing) but I feel our interactivity framework and practices must take into account that there are interactive environments that are not terminals and allow users who use those environments to have an acceptable and non-frustrating experience.

@ChiefMateStarbuck
Copy link
Contributor

Thank you for the PR @ddebarros and thank you for the feedback @joshuaauerbachwatson.

This PR opens up a larger question of how and if Doctl stays up to date with new UI CLI methodologies. At the same time, we cannot allow any breaking changes to Doctl that might increase churn or loss of revenue. Doctl runs on an unknown amount of systems, shells, and environments. With the current priorities of the company and team, we will have to revisit Doctl UI updates.

This is a great starting point @ddebarros and I look to merging this and similar PRs in the future. For today and for the foreseeable future, we will leave this PR open and unmerged.

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.

5 participants