PKI Keys List View #17239

Monkeychip · 2022-09-20T19:11:39Z

This is the PKI Keys List View.

This is a new endpoint so the adapter does not extend, similar to the Issuers adapter.

keys_list.mov

Notes:

waiting for some design feedback on the Empty State. Shouldn't block this PR.
Will merge the Certificates List View first, and then merge in main into this branch, just in case I've done any duplication clean up work in this PR.

Monkeychip · 2022-09-20T19:12:51Z

ui/app/models/pki/pki-issuer-engine.js

@@ -13,7 +13,7 @@ const validations = {
 };

 @withModelValidations(validations)
-export default class PkiIssuersEngineModel extends Model {
+export default class PkiIssuerEngineModel extends Model {


trying to make things consistent with the model class names. There is some inconsistency with PKI and in the API with plural vs. non plural. Doing my best to be consistent.

Monkeychip · 2022-09-20T19:14:22Z

ui/lib/core/addon/components/toolbar-link.js

+      return 'chevron-right'; // this is the default if no type is passed in.
+    } else {
+      return this.args.type;
+    }
  }


My thoughts here: if we want to make this logic better. A separate PR to revamp this param is needed. Which will impact everywhere (a lot of places) we use this as well as tests (I suspect). I can make a separate ticket to tackle this as a nice-to-do.

Ticket created.

hellobontempo

Some cleanup comments/questions!

hellobontempo · 2022-09-20T21:18:25Z

ui/app/adapters/pki/pki-key-engine.js

+
+  async query(store, type, query) {
+    const { backend, id } = query;
+    let response = await this.ajax(this.urlForQuery(backend, id), 'GET', this.optionsForQuery(id));


I think we can just return here and remove the async...await?

My understanding is that it's wrapping the ajax request in a promise—a good thing. It's essentially turning a sync call into an async call. I think it's a newer pattern for us. It's used in the named-path.js adapter query method and in the keymgmt/key adapter query method. Maybe a discussion for the team? I searched for some documentation on this, but couldn't find anything specific to our situation.

Ahh okay - I had just peeked at some older query methods and from what I could see we only make it async if we need to manipulate or filter the response in some way. And that the error/promise handling happened in the route, in the model hook here for example - but I could definitely be missing something!

I'll bring this up in our next standup. As I'm curious because the pattern isn't totally clear. Thanks for bringing it up.

hellobontempo · 2022-09-20T21:28:19Z

ui/lib/core/addon/components/toolbar-link.js

 * @param {boolean} disabled - pass true to disable link
 * @param {string} disabledTooltip - tooltip to display on hover when disabled
 */

 export default class ToolbarLinkComponent extends Component {
  get glyph() {
-    return this.args.type == 'add' ? 'plus' : 'chevron-right';
+    // not ideal logic, but updating old system to allow for other kinds of icons


I agree on separate PR as a nice cleanup to address why we are just changing the word 'add' to 'plus' 😅

But in the interim the following feels easier to read to me

get glyph() { const { type } = this.args; if (!type) return 'chevron-right'; return type === 'add' ? 'plus' : type; }

Thank you for the cleanup. You rock at the tight logic statements.

hellobontempo · 2022-09-20T21:30:46Z

ui/lib/pki/addon/templates/keys.hbs

+/>
+{{outlet}}


Same question here as the Issuer List PR

It's a great point. Will revisit after we get all the routes headers generated and the pattern becomes a little clearer.

hellobontempo · 2022-09-20T21:37:33Z

ui/app/models/pki/pki-key-engine.js

+import Model, { attr } from '@ember-data/model';
+
+export default class PkiKeyEngineModel extends Model {
+  @attr('string', { readOnly: true }) backend;


I may be reading the wrong docs - but should there also be a type and key_bits attribute? API Docs

(Disregard if this model is still WIP!)

You're not wrong, but yep, model is still a WIP. I only added the read and list params here. Something I've been doing for these list views. I should have left a comment about it.

hellobontempo · 2022-09-20T21:41:50Z

ui/lib/pki/addon/templates/keys/index.hbs

+            </span>
+            <div class="is-flex-row has-left-margin-l has-top-margin-xs">
+              {{#if model.isDefault}}
+                <span class="tag has-text-grey-dark">default issuer</span>


should this read default key?

Good catch. I believe so, I'll also confirm with design. She actually didn't have this param on the designs, she had keyName, which is not available to us on the LIST endpoint.

I'll go ahead and remove this until I get the response from the backend and design. It could be the default issuer because I don't know of a parma where is_default relates to a key. So maybe it's a key associated with the default issuer. Either way, it should be removed until clarified.

Gotcha - I had interpreted this bit from the docs as the default key name was default

key_name (string: "") - When a new key is created with this request, optionally specifies the name for this. The global ref default may not be used as a name.

hellobontempo · 2022-09-20T21:45:27Z

ui/lib/pki/addon/templates/keys/index.hbs

+              {{model.id}}
+            </span>
+            <div class="is-flex-row has-left-margin-l has-top-margin-xs">
+              {{#if model.isDefault}}


Curious, where does this attribute come from? I noticed it on the model, but not in the docs. I may be missing context, but could we instead check for if (eq model.keyName "default)

It comes from the LIST network request. So similar to how you had to clients serializer it gets added to the model from what is returned on the LIST endpoint. In this view we only have the model data built out from what is returned on the LIST endpoint. It's not the full model for the key, so keyName isn't available to us. When we go to the edit/read/update views we'll then have the model fully built out for the key.

Here is the LIST response.

As an aside: the payload returned is slightly different than the docs. I have a question out about that. E.g. it shows the param is_default when the payload on the docs does not show that.

Thank you for the screenshot! Super helpful. This makes sense now

setup

24eeb66

Monkeychip added ui pr/no-changelog labels Sep 20, 2022

Monkeychip commented Sep 20, 2022

View reviewed changes

cleanup

f22b710

hellobontempo approved these changes Sep 20, 2022

View reviewed changes

Monkeychip added 2 commits September 20, 2022 17:19

cleanup

f7ed4c4

Merge branch 'main' into ui/VAULT-6518/pki-keys-list-view

3c67b65

vercel bot deployed to Preview September 20, 2022 23:27 View deployment

Monkeychip merged commit 613498c into main Sep 21, 2022

Monkeychip deleted the ui/VAULT-6518/pki-keys-list-view branch September 21, 2022 14:41

hashishaw mentioned this pull request Jan 27, 2023

UI: PKI Sign Intermediate #18842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKI Keys List View #17239

PKI Keys List View #17239

Monkeychip commented Sep 20, 2022 •

edited

Loading

Monkeychip Sep 20, 2022

Monkeychip Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo left a comment

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

hellobontempo Sep 20, 2022

Monkeychip Sep 20, 2022

Monkeychip Sep 20, 2022

hellobontempo Sep 20, 2022

		/>
		{{outlet}}

PKI Keys List View #17239

PKI Keys List View #17239

Conversation

Monkeychip commented Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip commented Sep 20, 2022 •

edited

Loading