UI: Migrate <ConfigureAwsSecret /> to HDS & TS#27367
Conversation
|
CI Results: |
| await click('[data-test-secret-backend-configure]'); | ||
|
|
||
| assert.strictEqual(currentURL(), `/vault/settings/secrets/configure/${path}`); | ||
| assert.ok(findAll('[data-test-aws-root-creds-form]').length, 'renders the empty root creds form'); |
There was a problem hiding this comment.
assert.ok --> assert.dom because assert.ok cannot be trusted 🙅
| assert.ok( | ||
| find('[data-test-flash-message]').textContent.trim(), | ||
| `The backend configuration saved successfully!` | ||
| assert.true( |
There was a problem hiding this comment.
the previous assertions would've passed even if the same flash message rendered, so this guarantees that a fresh message is rendered.
| <T.Tab data-test-tab="access-to-aws"> | ||
| Dynamic IAM root credentials | ||
| </T.Tab> | ||
| <T.Tab data-test-tab="lease"> |
There was a problem hiding this comment.
one thing to note -- this page was previously using query params to keep track of which tabs were selected. if this is helpful for ensuring users can copy/paste a URL with a tab pre-selected i can totally add that back in, just wanted to make sure it was necessary first. 🙂
There was a problem hiding this comment.
I think the new designs Yankun is working on will remove tabs all together. That aside, I think removing queryParams here is okay because given the fact this page never properly transitioned after save I suspect it's not being used as much as we'd hope.
There was a problem hiding this comment.
ah, good to know! should i look into fixing the transition?
There was a problem hiding this comment.
Two thoughts on the transition:
- we make a pr that just fixes the transition and can be backported (so it couldn't depend on any changes here).
- we address the transition here or a future pr and don't backport it. Nobody has complained.
I think ideally, we'd do number 1, but only if it was easy to both fix and backport—otherwise, we can proceed with a fix in this aws 1.18.x work.
| <T.Panel> | ||
| <form | ||
| onsubmit={{action | ||
| "saveRootCreds" |
There was a problem hiding this comment.
I'd say wait for the new designs first. We just spoke in standup about how KVv2 create also makes two network requests on save. I think it's in the same component but rendered conditionally. Also, something to keep in mind, do we need to make new components if they're not consumed elsewhere or is one configuration create component sufficient and separates the concerns clearly?
There was a problem hiding this comment.
I'd say wait for the new designs first.
ah mk, if designs are going to change i can hold off, although i do already have PRs ready for them (see links in my first comment) 😅
We just spoke in standup about how KVv2 create also makes two network requests on save. I think it's in the same component but rendered conditionally.
whatcha mean by this? this component is only used when configuring an AWS secret engine. is there something else behind the scenes i'm missing?
do we need to make new components if they're not consumed elsewhere or is one configuration create component sufficient and separates the concerns clearly?
very fair question! there are a couple reasons why i often opt for splitting components up, even if they're not reused:
-
enforcing separation of concerns: if a component starts to handle multiple responsibilities (e.g., complex logic, numerous conditionals, and various arguments), it's an indicator that it might benefit from being split into smaller, more focused components. this makes each component simpler and easier to understand.
-
supporting readability and maintainability: when each component has a clear purpose, other developers (or my future self) can more easily understand what it does. this readability makes maintaining and updating the component simpler and less error-prone.
-
testing (this is usually my biggest motivator): components with too many responsibilities can become hard to test. writing comprehensive tests for a single, monolithic component requires covering a wide range of scenarios, which can be complex and time-consuming. by splitting components, you can create more targeted tests for each component's specific functionality, leading to more reliable and maintainable tests.
There was a problem hiding this comment.
whatcha mean by this? this component is only used when configuring an AWS secret engine. is there something else behind the scenes i'm missing?
If we move away from these tabs and have only one view, then this will become a similar scenario to KVv2 create. Chelsea brought this up as example—it's just another create page that makes two network requests and both endpoints have different permission checks because they're different paths—similar to aws configure with /root vs /leases. It was just an example we could turn to if we needed to see how we handled a similar situation.
Yankun is working on the designs today so they should be done soon but I suspect we'll move to one create view with now tabs.
And for splitting up in components, thank you for the thoughts. Testing is a great point, and with that in mind splitting into two components makes sense. I'll check out your current WIP prs and any thoughts and feelings I'll put there, but please feel free to continue on that work.
|
Build Results: |
Monkeychip
left a comment
There was a problem hiding this comment.
A couple of non-blocking comments, more for the next chunk of work.
|
Also, let's change the milestone to 1.18.0. At 1.17.1 it indicates we'll need to backport to 1.17.x and we don't really backport feature changes to minor patches. |
4ea66d5 to
52af959
Compare
🛠️ Description
While prototyping WIF work I found a couple improvements that'll make WIF work easier. This pr:
<ConfigureAwsSecret />to useHds::Tabs🔗 Links
#27342
See WIF UI whimsical
📸 Screenshots
🏗️ How to Build and Test the Change