[Secrets Sync] enable access to Sync clients page for HVD clusters #26713
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: failed ❌ |
andaley
changed the title
andaley
changed the base branch from
main
to
ui/VAULT-25468/enable-secrets-sync-hvd
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
a4f86e0 to
1717654
Compare
| @@ -151,32 +151,7 @@ | |||
| </Hds::Alert> | |||
| {{/if}} | |||
|
|
|||
| <nav class="tabs has-bottom-margin-s" aria-label="navigation for managing client counts"> | |||
There was a problem hiding this comment.
this file & its associated test were getting a bit hairy, so moved it into a separate component 🧹
| @@ -40,12 +40,6 @@ module('Acceptance | clients | overview', function (hooks) { | |||
| timestamp.now.restore(); | |||
| }); | |||
|
|
|||
| test('it should render the correct tabs', async function (assert) { | |||
There was a problem hiding this comment.
this is now covered by the nav-bar-tests integration tests 😀
integration > acceptance tests for basic "does this thing show up on the DOM?"
| // we can't tell if HVD clusters have the feature or not, so we show it by default | ||
| // if the cluster is not HVD, show the tab if the feature is on the license |
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
375f891 to
2e460d0
Compare
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
dc85aad to
36958d6
Compare
| 'Secret sync': formatNumber([topNamespace.secret_syncs]), | ||
| }; | ||
| for (const label in expectedStats) { | ||
| module('filtering charts', function () { |
There was a problem hiding this comment.
Was the decision to convert to modules to try and address the flakiness? In my experience, these tests weren't flaky before 🤔 , so if that isn't the case then could this refactor happen in a separate PR?
I completely agree with refactoring these into modules! But a separate PR would be make this one easier to review and check that test coverage has been added for the relevant logic updates. Historically, we've also had folks accidentally remove important test assertions for this area of the code base due to the intricacies of this dang dashboard 😞
There was a problem hiding this comment.
that's correct, i moved them into separate tests to try to isolate the failures. what happened here was that i switched to using the sync mirage handler instead of the clients one at the top-level beforeEach hooks because making assertions on the secrets sync charts required secrets sync being included in the /activated-features response, which the clients handler didn't have. similarly, i assumed the sync handler would also be better to use here since it would include sync client data to populate the charts. however when i made this switch, the tests failed inconsistently, so i split the tests up to try and figure out if it was only specific aspects of the tests or if it was the whole ding-dang thing. 😂
as it turns out, the root of the problem lies somewhere in the differences between the sync and clients handlers, because yeah, splitting up the module didn't help much at all. i am reverting this change today and am investigating other options (potentially leaving the sync assertions out of these tests and instead making those assertions in a completely separate module so we can use the sync handler instead of clients). 🤔
There was a problem hiding this comment.
i looked at this for quite awhile today and these chart / chart filtering tests seem to be flaky; at least on our sidebranch. check out the video i left in slack for more (github wouldn't let me upload it here because the file size was too big 🙁 )
@Monkeychip and i talked about it while pairing today and assuming these tests are failing on other branches, we'll need to tackle the fixes in a separate PR.
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
64ef255 to
2a489c0
Compare
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
2a489c0 to
d603f00
Compare
| this.server.get('/sys/activation-flags', () => ({ | ||
| data: { activated: ['secrets-sync'], unactivated: [] }, | ||
| })); | ||
| syncHandler(this.server); |
There was a problem hiding this comment.
the sync handler sets up all the mocked endpoints that we had before anyway 😀
| .dom(CLIENT_COUNT.statTextValue('Secret sync')) | ||
| .doesNotExist('stat is hidden because feature is not activated'); | ||
| assert.dom(CLIENT_COUNT.statTextValue('Entity')).exists('other stats are still visible'); |
There was a problem hiding this comment.
we were previously making a doesNotExist assertion on the wrong element. oops! i fixed that throughout this file and added a related assertion to ensure we don't get a falsely passing tests (i.e. if all the chart stats were missing for some reason).
There was a problem hiding this comment.
Whoops - probably my bad when I last refactored these to clean them up a bit. Great catch!
| @@ -26,22 +27,9 @@ module('Integration | Component | clients | Clients::Page::Sync', function (hook | |||
| setupMirage(hooks); | |||
|
|
|||
| hooks.beforeEach(async function () { | |||
| clientsHandler(this.server); | |||
There was a problem hiding this comment.
i promise i didn't actually delete this! i just needed to split the tests out into two modules because i needed to use the syncHandler for some tests and no handler for another. mirage doesn't behave well if you try to switch between handlers within the same module, nor does it work when you try to overwrite a handler already in use. therefore it was easiest to split these up.
the whitespace here makes it look like a much bigger change; i recommend using github's "hide whitespace" feature to view this diff more easily.
…or feature is activated
…e to be/ activated
…ashicorp/vault into ui/vault-26096/enable-sync-hvd-clients-page
andaley
force-pushed
the
ui/vault-26096/enable-sync-hvd-clients-page
branch
from
04e3328 to
64d5338
Compare
|
Build Results: |
| @@ -162,6 +164,22 @@ export default class ClientsCountsPageComponent extends Component<Args> { | |||
| return activity?.total; | |||
| } | |||
|
|
|||
| get showSecretsSync(): boolean { | |||
There was a problem hiding this comment.
since this value is only needed on one component, we don't need to compute this in the route to add to the model. better off calculating it closer to where we actually use the value, where we already have enough context to compute it. in other words, by calculating the value here we can simply pass it straight into the <NavBar /> which is the only place where it's used. 😀
| return { | ||
| activity, | ||
| activityError, | ||
| config, | ||
| endTimestamp, | ||
| isSecretsSyncActivated, |
There was a problem hiding this comment.
Great idea moving this to the counts.ts component! Much clearer there than passing down from this route model hook 🥇
| test('it should hide secrets sync charts', async function (assert) { | ||
| assert.dom(CHARTS.chart('Secrets sync usage')).doesNotExist(); | ||
| test('it should hide secrets sync stats', async function (assert) { | ||
| assert.dom(CLIENT_COUNT.statTextValue('Secret sync')).doesNotExist(); |
There was a problem hiding this comment.
In case it comes up in another review - this stat was intentionally labelled Secret sync (singular) because it's referring to the client type, not the secrets sync feature.
andaley
merged commit 86430f9
into
ui/VAULT-25468/enable-secrets-sync-hvd
* Allow Managed clusters to see Secrets Sync Overview and Sidebar nav (#26649) * update badge text and allow hvd on secrets sync views * update logic in Secrets Sync overview and cta for hvd. * spacing * rearrange based on pr feedback * fix return on badgeText and cluster nav test * fix landing cta tests * update test to reflect new changes * moved call to feature-flags from application route to the service to match patterns * add managed test coverage on overview component test and remove premium feature so cta message appplies to both managed and non-managed clusters * missed service name and unskip admin test * clean up * fix tests * flags test fix * Rename isManaged and managedNamespaceRoot (#26697) * renames * lowercase HVD to match * missed some * test failure * [Secrets Sync] enable access to Sync clients page for HVD clusters (#26713) * feat: split client counts navbar into separate component * acceptance/clients/counts/overview-test: remove tests now covered by int tests * clients counts route: rename isSecretsSyncActivated to showSecretsSync * sync clients page: show unactivated state unless sync client history or feature is activated * client counts navbar: show sync tab only if client history or is /able to be/ activated * clients overview page: only show sync charts if activated * fix: rename isManaged to isHvd * acceptance/counts/overview-test: add HVD tests * acceptance/counts/overview-test: clean up unused cruft * aceptance/clients/counts/overview-test: ensure we dont get false negatives * chore: move Clients::Error to Clients::Counts::Error * chore: calculate showSecretSync in page component instead of route * chore: add copyright headers * acceptance/clients/counts/overview-test: stub activated flags to fix test * [Secrets sync] update sync test selectors (#26824) * acceptance/clients/counts/overview-test: use imported test selectors * general-selectors: add missing emptyStateSubtitle property * acceptance/clients/counts/sync: nest tests in top level module for easier test runs * Add permissions check to show/hide activate button (#26840) * add permissions check to flags service and consume in overview template * add back missing refresh * fix test failures * add test coverage * clean up * address flaky test * grr * address test failures * add changelog * try to fix test failure only on gh * fix fetch to match previous implementation of feature-flags * fix failing test * update comment --------- Co-authored-by: Noelle Daley <[email protected]> Co-authored-by: [email protected] <[email protected]>
* Allow Managed clusters to see Secrets Sync Overview and Sidebar nav (hashicorp#26649) * update badge text and allow hvd on secrets sync views * update logic in Secrets Sync overview and cta for hvd. * spacing * rearrange based on pr feedback * fix return on badgeText and cluster nav test * fix landing cta tests * update test to reflect new changes * moved call to feature-flags from application route to the service to match patterns * add managed test coverage on overview component test and remove premium feature so cta message appplies to both managed and non-managed clusters * missed service name and unskip admin test * clean up * fix tests * flags test fix * Rename isManaged and managedNamespaceRoot (hashicorp#26697) * renames * lowercase HVD to match * missed some * test failure * [Secrets Sync] enable access to Sync clients page for HVD clusters (hashicorp#26713) * feat: split client counts navbar into separate component * acceptance/clients/counts/overview-test: remove tests now covered by int tests * clients counts route: rename isSecretsSyncActivated to showSecretsSync * sync clients page: show unactivated state unless sync client history or feature is activated * client counts navbar: show sync tab only if client history or is /able to be/ activated * clients overview page: only show sync charts if activated * fix: rename isManaged to isHvd * acceptance/counts/overview-test: add HVD tests * acceptance/counts/overview-test: clean up unused cruft * aceptance/clients/counts/overview-test: ensure we dont get false negatives * chore: move Clients::Error to Clients::Counts::Error * chore: calculate showSecretSync in page component instead of route * chore: add copyright headers * acceptance/clients/counts/overview-test: stub activated flags to fix test * [Secrets sync] update sync test selectors (hashicorp#26824) * acceptance/clients/counts/overview-test: use imported test selectors * general-selectors: add missing emptyStateSubtitle property * acceptance/clients/counts/sync: nest tests in top level module for easier test runs * Add permissions check to show/hide activate button (hashicorp#26840) * add permissions check to flags service and consume in overview template * add back missing refresh * fix test failures * add test coverage * clean up * address flaky test * grr * address test failures * add changelog * try to fix test failure only on gh * fix fetch to match previous implementation of feature-flags * fix failing test * update comment --------- Co-authored-by: Noelle Daley <[email protected]> Co-authored-by: [email protected] <[email protected]>
🛠️ Description
🧹 move client count nav bar to its own component
📸 Screenshots
🏗️ How to Build and Test the Change
For OSS
For HVD (most easily accomplished by pulling this down locally and hard-coding
isHvdManaged = truein theflagsservice)