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

UI/browser console update #20590

Merged
merged 12 commits into from
May 17, 2023
Merged

UI/browser console update #20590

merged 12 commits into from
May 17, 2023

Conversation

hashishaw
Copy link
Contributor

@hashishaw hashishaw commented May 15, 2023

This PR updates the API Web Console in the following ways:

Add a kv-get command which will read the data or metadata of a KV v2 secret

The Vault CLI has a kv subcommand for managing and reading KV v2 secrets, but in the UI Web CLI the user must know the full API path that the kv commands map to.

Before in Web CLI

# read data for secret/foo
read secret/data/foo
# read metadata for secret/foo
read secret/metadata/foo

After in Web CLI

# read data for secret/foo
kv-get secret/foo
# read metadata for secret/foo
kv-get secret/foo -metadata

Fix the Open API Explorer filter input

We recently discovered that the input would blur each time you type a letter, which is a terrible user experience. The fix present in this PR executes the path filtering in-page on input and updates the namespace param on change (once the input is blurred)

Allow page to scroll fully when console panel is open

Before, if you had the console panel open and the page behind it had content that fell behind the console, it was inaccessible. Now, it adds padding to the main frame when the console is open so that the page is fully scrollable.

Screenshot 2023-05-15 at 2 53 48 PM

@hashishaw hashishaw added the ui label May 15, 2023
@hashishaw hashishaw added this to the 1.14 milestone May 15, 2023
@hashishaw hashishaw marked this pull request as ready for review May 15, 2023 19:59
// will be "key=value" or "foo=bar=baz"
// split on the first =
// default to value of empty string
const [item = '', value = ''] = val.split(/=(.+)?/);
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know you can default values when destructuring! Super cool!

interface CommandLog {
type: string;
content?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea to put the interface type definitions before each function :) I think it makes it more intuitive to know what types are needed for the specific function.

@@ -125,6 +125,10 @@ $console-close-height: 35px;
min-height: 400px;
}

.main--console-open {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 this a great css pattern that we learned from the offsite! should we use this as a standard pattern for the future? https://css-tricks.com/bem-101/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was inspired by the offsite discussion! I think that is the plan although I know some of our CSS patterns are still in the air.

@@ -11,7 +11,8 @@
<div class="field is-marginless">
<p class="control has-icons-left">
<input
oninput={{queue (action "updateFilter") (action "proxyEvent")}}
oninput={{action "proxyEvent"}}
onchange={{action "updateFilter"}}
Copy link
Contributor

@kiannaquach kiannaquach May 15, 2023

Choose a reason for hiding this comment

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

same here

Suggested change
onchange={{action "updateFilter"}}
{{on "change" (action "updateFilter")}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can, I was trying to change this file as little as possible, since it will need a glimmer refresh in the future. This is also the basic <input> rather than the ember's built-in so I'll go ahead and see if that will upgrade smoothly too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I was able to update the action to the newer syntax, but the input broke in the same way as before when I tried to update it to the builtin component 🤔 Shipping without that update for now

@@ -6,6 +6,7 @@

Commands:
read Read data and retrieves secrets
kv-read Read data for kv v2 secret engines. Use -metadata flag to read metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious of the reason behind deviating from the vault CLI commands and using read here instead of get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the existing conventions on the web CLI, but open to changing. It already deviates quite a bit from the CLI

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's a good point about the kv commands. I think as you have it now is reasonable! You made some nice improvements to the help command so I think it's clear what the commands do

Copy link
Contributor

Choose a reason for hiding this comment

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

I think kv-read aligns nicely with the current web-cli commands. It would be great if all the commands directly mapped to the cli commands but since that is not the case then I think with the examples and the help command users have a good reference.

<input
oninput={{queue (action "updateFilter") (action "proxyEvent")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside - I wonder if we should check for other queue actions in the codebase - was this causing the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not by virtue of using the queue, but the proxyEvent was causing the input to lose focus, which we only want to happen once the user is done typing

Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

I pulled down the branch to test and all the changes look good. Nice work 👏. 🚢

@@ -39,7 +39,7 @@
</:footer>
</Hds::SideNav>
</Frame.Sidebar>
<Frame.Main id="app-main-content">
<Frame.Main id="app-main-content" class={{if this.console.isOpen "main--console-open"}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Comment on lines 12 to 19
<p class="has-text-grey is-font-mono has-bottom-margin-s">Examples:</p>
<p class="has-text-grey is-font-mono">→ Write secrets to kv v1: write &lt;mount&gt;/my-secret foo=bar</p>
<p class="has-text-grey is-font-mono">→ List kv v1 secret keys: list &lt;mount&gt;/</p>
<p class="has-text-grey is-font-mono">→ Read a kv v1 secret: read &lt;mount&gt;/my-secret</p>
<p class="has-text-grey is-font-mono">→ Mount a kv v2 secret engine: write sys/mounts/&lt;mount&gt; type=kv
options=version=2</p>
<p class="has-text-grey is-font-mono">→ Read a kv v2 secret: kv-read &lt;mount&gt;/secret-path</p>
<p class="has-text-grey is-font-mono">→ Read a kv v2 secret's metadata: kv-read &lt;mount&gt;/secret-path -metadata</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Having examples is helpful 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Do we want to add somewhere that the commands listed are the ONLY ones we support?

@hashishaw hashishaw force-pushed the ui/console-update branch from 0dfc1f4 to d0551ca Compare May 17, 2023 16:03
@hashishaw hashishaw merged commit 7c66970 into main May 17, 2023
@hashishaw hashishaw deleted the ui/console-update branch May 17, 2023 16:41
@hellobontempo hellobontempo changed the title UI/console update UI/browser console update Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants