UI: Upgrade to Ember 4.12 #22122

hashishaw · 2023-07-28T17:42:15Z

This PR upgrades from 4.4 to 4.12, the latest LTS.

Initially we were going to upgrade to 4.8, but we ran into many stable identifier issues in tests and in the app. After bumping to 4.12 instead (see #21955 for comments) the app and tests are more stable.

…e changes

Attempting to unload records after test teardown (as initiated in the cluster route deactivate hook) results in error like: Promise rejected after "test name": Assertion Failed: Expected a stable identifier reference: https://medium.com/@jenweber/help-my-ember-acceptance-tests-are-leaking-2e48a2e2dce5

- default serializer makes array values empty - assign this.targets to self so that the array tracks updates

- willDestroy on component called after test unload, which destroys the store - wrapped model unload call in try/catch so the error doesn't bork the test

* fixes store service unload method overrides * bumps ember to 4.12 * removes unload method overrides in store service * bumps ember test-helpers and qunit back down * fixes memory leaks in tests when rolling back attributes in willDestroy hook * removes unnecessary logouts from acceptance tests * updates deprecation filter to also target 5.0 as well as 5.0.0 version strings * removes model.destroy from unload-model-route mixin * removes secret-engine and auth-method unloadAll from routes causing issues in tests * removes mount-config serializer * converts replication-mode service to native class and makes mode a tracked property -- fixes template not updating when switching between dr and performance replication routes * updates code-mirror modifier to new api -- fixes issue with editor not installing on element * fixes pki key issue rolling back attributes after destroyRecord has been called * fixes enterprise-oidc-namespace test * adds comment to mfa-form test regarding possible timer issues * pins node version to 16 * fixes expected stable identifier issue in tests * adds destroy checks to ObjectProxy in maybe-query-record macro * fixes issues with v2 secrets * revert concurrently for npm-run-all * updates yarn lock * fixes js lint errors * fixes hbs lint errors * fix dupe accessor on secret-engine model * Move isV2 to getter on secret-edit --------- Co-authored-by: Chelsea Shaw <cshaw@hashicorp.com>

This reverts commit 91df1a0.

hashishaw · 2023-07-28T19:07:49Z

ui/app/components/identity/edit-form.js

      model.rollbackAttributes();
    }
+    this._super(...arguments);


Always call this._super(...) last in willDestroy otherwise we get errors in tests

hashishaw · 2023-07-28T21:09:03Z

ui/app/components/database-role-edit.js

Database role tests were exiting early, so this was an attempt to stabilize those

ui/app/components/mfa/mfa-login-enforcement-form.js

hashishaw · 2023-07-31T14:13:11Z

ui/app/initializers/deprecation-filter.js

@@ -10,10 +10,10 @@ export function initialize() {
  registerDeprecationHandler((message, options, next) => {
    // filter deprecations that are scheduled to be removed in a specific version
    // when upgrading or addressing deprecation warnings be sure to update this or remove if not needed
-    if (options?.until !== '5.0.0') {
-      next(message, options);
+    if (options?.until.includes('5.0')) {


deprecations are returning 5.0 and 5.0.0

hashishaw · 2023-07-31T14:31:19Z

ui/app/templates/vault/cluster/secrets/backends.hbs

@@ -78,7 +78,7 @@
      </div>
      {{#if backend.accessor}}
        <code class="has-text-grey is-size-8">
-          {{backend.accessor}}
+          {{if (eq backend.version 2) (concat "v2 " backend.accessor) backend.accessor}}


No longer conflating accessor with the version

hashishaw · 2023-07-31T14:31:25Z

ui/config/ember-cli-update.json

@@ -3,7 +3,7 @@
  "packages": [
    {
      "name": "ember-cli",
-      "version": "4.4.0",
+      "version": "4.12.1",


hashishaw · 2023-07-31T14:31:57Z

ui/lib/core/addon/components/navigate-input.hbs

@@ -1,7 +1,6 @@
 <div class="navigate-filter">
  <div class="field" data-test-nav-input>
    <p class="control has-icons-left">
-      {{! template-lint-disable no-down-event-binding }}


No longer a rule 🎉

hashishaw · 2023-07-31T14:32:06Z

ui/lib/core/addon/components/ttl-picker.js

@@ -45,7 +45,6 @@ export default class TtlPickerComponent extends Component {
  @tracked recalculateSeconds = false;
  @tracked time = ''; // if defaultValue is NOT set, then do not display a defaultValue.
  @tracked unit = 's';
-  @tracked recalculateSeconds = false;


duplicate attr

hashishaw · 2023-07-31T14:33:54Z

ui/lib/core/addon/modifiers/code-mirror.js

-    }
-    if (this._editor.getValue() !== this.args.named.content) {
-      this._editor.setValue(this.args.named.content);
+  modify(element, positionalArgs, namedArgs) {


Codemirror API changed -- see commit for context

hashishaw · 2023-07-31T14:35:46Z

ui/tests/acceptance/access/namespaces/index-test.js

@@ -19,10 +18,6 @@ module('Acceptance | Enterprise | /access/namespaces', function (hooks) {
    return authPage.login();
  });

-  hooks.afterEach(function () {


authPage.login logs out before each test, so logout after each is unnecessary.

hashishaw · 2023-07-31T14:38:56Z

ui/tests/acceptance/mfa-login-test.js

+  });
+  hooks.afterEach(function () {
+    // Manually clear token after each so that future tests don't get into a weird state
+    this.auth.deleteCurrentToken();


This was a weird one. If I tried to visit logout in the afterEach hook, one of these tests would fail because it queried the MFA verification more than once. Rather than open that can of worms this fixes the test for now, but we should double-verify with smoke tests that the MFA flow is working as intended. In any case, even if the flow is making multiple requests that shouldn't break the flow

hashishaw · 2023-07-31T14:39:17Z

ui/tests/acceptance/oidc-config/clients-assignments-test.js

@@ -34,15 +33,11 @@ module('Acceptance | oidc-config clients and assignments', function (hooks) {
    ENV['ember-cli-mirage'].handler = 'oidcConfig';
  });

-  hooks.beforeEach(async function () {
-    this.store = await this.owner.lookup('service:store');


this.owner.lookup isn't a promise

hashishaw · 2023-07-31T14:41:03Z

ui/tests/acceptance/secrets/backend/database/secret-test.js

    ]);
-    const token = consoleComponent.lastTextOutput;
-    await logout.visit();
+    await navToConnection(backend, connection);


Visiting the connection page directly was not re-querying the capabilities correctly. I was unable to replicate this behavior in the app, so navigating via the UI seems to resolve the test issue

hashishaw · 2023-07-31T14:41:45Z

ui/tests/acceptance/secrets/backend/kv/secret-test.js

@@ -682,7 +681,8 @@ module('Acceptance | secrets/secret/create, read, delete', function (hooks) {
    await deleteEngine(enginePath, assert);
  });

-  test('version 2: with metadata no read or list but with delete access and full access to the data endpoint', async function (assert) {
+  // TODO VAULT-16258: revisit when KV-V2 is engine
+  test.skip('version 2: with metadata no read or list but with delete access and full access to the data endpoint', async function (assert) {


This one is being all sorts of weird. But a use case to make sure we cover in the KV engine work.
I did test in the app and it seemed to work as intended

hashishaw · 2023-07-31T14:43:48Z

ui/tests/helpers/commands.js

Added this test helper so we can quickly make things happen in the app that we're not testing the workflow for exactly, but nonetheless need them to happen for the test scenario. I expect this to grow over time! If anyone has suggestions for clearer verbage please comment 🙏

github-actions · 2023-07-31T15:03:00Z

Build Results:
All builds succeeded! ✅

github-actions · 2023-07-31T15:06:00Z

CI Results:
All Go tests succeeded! ✅

hellobontempo · 2023-07-31T15:45:56Z

ui/app/components/auth-form.js

@@ -219,6 +219,8 @@ export default Component.extend(DEFAULTS, {
            };
          })
        );
+        // without unloading the records there will be an issue where all methods set to list when unauthenticated will appear for all namespaces


Is this because of the upgrade? I thought since we sent the namespace as the header, the /sys/internal/ui/mounts requests only fires for that namespac

Right, but we have to unload the store here or else that call will just append to the store we already have. This note is just to clarify what the below is doing, so that we can remove it later (unloadAll was causing issues throughout tests)

Ahh I see - I thought the store cleared with the new request. Thanks for clarifying!

ui/app/components/mfa/mfa-login-enforcement-form.js

hellobontempo

some comments so far! (note to self, left off at 65/113 files)

hellobontempo · 2023-07-31T16:31:17Z

ui/app/macros/maybe-query-record.js

@@ -7,6 +7,23 @@ import { computed } from '@ember/object';
 import ObjectProxy from '@ember/object/proxy';
 import PromiseProxyMixin from '@ember/object/promise-proxy-mixin';
 import { resolve } from 'rsvp';
+/**
+ * after upgrading to Ember 4.12 a secrets test was erroring with "Cannot create a new tag for `<model::capabilities:undefined>` after it has been destroyed"


I know this is an error I've also seen while developing - I wonder if the test was finally catching it now after the upgrade? Or do you think this behavior is just isolated to tests?

could be, but I thought in the app we saw an id instead of undefined 🤔 FWIW tests were failing with A record in a disconnected state cannot utilize the store. This typically means the record has been destroyed, most commonly by unloading it. when I ran the test suite reverting this change but keeping all others.
The good/bad news is that we aren't the only ones seeing this. But, it seems to be most problematic in tests but making a note here to test the application with varying capabilities to make sure workflows aren't broken with this change

Ah yeah, that error is familiar too. Sounds good!

hellobontempo · 2023-07-31T16:33:19Z

ui/app/models/cluster.js

-    return !!this.nodes.findBy('sealed', false);
-  }),
+  get unsealed() {
+    return !!this.nodes.find((node) => {


.findBy is still an ember method, right? No need to change - just curious! 😄

Just checked -- it is! It isn't an MDN Array method though, which is probably what I was thinking of

Yep - it's a special ember function from what I remembered

hellobontempo · 2023-07-31T16:34:59Z

ui/app/models/cluster.js


  //replication mode - will only ever be 'unsupported'
  //otherwise the particular mode will have the relevant mode attr through replication-attributes
-  mode: attr('string'),
-  allReplicationDisabled: and('{dr,performance}.replicationDisabled'),


I'm so glad this syntax isn't a thing in octane 😅

hellobontempo · 2023-07-31T16:36:53Z

ui/app/models/cluster.js

-  replicationAttrs: computed('dr.mode', 'performance.mode', 'replicationMode', function () {
+    return !this.dr?.mode || !this.performance?.mode;
+  }
+  get replicationAttrs() {
    const replicationMode = this.replicationMode;
    return replicationMode ? get(this, replicationMode) : null;


should this get be updated? or can it remain get()

Suggested change

return replicationMode ? get(this, replicationMode) : null;

return replicationMode ? this.replicationMode : null;

I tried that, and since replicationMode is a dynamic string get is the only way i got this to work. It would have to be something like this[replicationMode] which, in testing, didn't work. I also learned that get is the only way to get nested paths, so if something was at foo.bar.baz you could do get(foo, "bar.baz") but not foo["bar.baz"]

Ah okay - thanks for explaining!

hellobontempo · 2023-07-31T16:42:11Z

ui/app/serializers/mfa-login-enforcement.js

@@ -34,12 +34,13 @@ export default class MfaLoginEnforcementSerializer extends ApplicationSerializer
    }
    return payload;
  }
-  serialize() {
+  serialize(snapshot) {


Why does snapshot need to be passed in here? This is also something caught by the pki role serializer here

snapshot is used on line 41-42, because the serialized value json was empty for those arrays 😕 I can investigate this again, but I assumed when I made this change that something changed in the ember-data serializer. I'll check PKI as well, since there might not have been a test that caught that one

Ahh okay. For added context, I remember that when those values were serialized, the keys were removed all together so checking that the key existed on the json was sufficient, because if it didn't exist then it meant it was empty.

If the json value exists and is empty for those arrays now, then that means we can probably remove this code all together as it sounds like the problem may have been fixed on an ember data level ?!

ohh interesting -- turns out the component was using addObject on the model attribute which is supposed to be an array, but using that method instead of setting it to something directly (model.thing = thing) doesn't trigger an update to the snapshot that gets passed into the serializer 😱 Looking at PKI, the component does set it directly, so I'm updating the MFA component to set it like that instead.

hellobontempo · 2023-07-31T16:49:05Z

ui/app/templates/vault/cluster/secrets/backend/metadata.hbs

-      {{/if}}
-    </EmptyState>
-  {{/each-in}}
+  {{else}}{{#if this.noReadAccess}}


Oof, I think this conditional arrangement is something that we improved when Angel updated the KV engine metadata read page

hellobontempo · 2023-07-31T17:19:47Z

ui/tests/acceptance/secrets/backend/database/secret-test.js

+      assert.strictEqual(currentURL(), `/vault/secrets/${backend}/list`, 'Cancel button links to list view');
+    });
+  }
+  test('database connection create and edit: vault-plugin-database-oracle', async function (assert) {


hellobontempo · 2023-07-31T17:22:01Z

ui/tests/helpers/commands.js

zofskeez

🎉 🚀 🚢
LGTM. Nothing stands out from the review but I think the sooner we can get this merged for use in development the better to flush out any bugs that may still be lurking.

hashishaw and others added 19 commits July 28, 2023 12:36

ember-cli-update ran

09ce409

fix incorrect seal status - update clusterModel into class-based

b6812d3

Fix activeCluster not being re-calculated when the store cluster valu…

da76d64

…e changes

Dont call window.confirm if testing

082e603

Prevent test failure from undefined leaderNode

bf50b35

Fix oidc/* test helper - direct access instead of .content

f894d2c

secret-edit -- continue transition if model not clean or deleted

b45d545

Fix fetchPage test (unloadAll fixed)

bdfb09b

Fix MFA login enforcement form test

92461ca

- default serializer makes array values empty - assign this.targets to self so that the array tracks updates

Fix mount-backend-form test

199460a

- willDestroy on component called after test unload, which destroys the store - wrapped model unload call in try/catch so the error doesn't bork the test

stabilize has-permission-test

2826315

this.owner.lookup is not a promise

fa692ba

transition to logout as intended even if not authed

4231802

Revert "transition to logout as intended even if not authed"

29f0d51

This reverts commit 91df1a0.

transition to logout if intended and not authed

af00582

Add console commands test helper

e452be2

Fix database tests

b19cdce

hashishaw added the ui label Jul 28, 2023

hashishaw added this to the 1.15 milestone Jul 28, 2023

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 28, 2023

hashishaw mentioned this pull request Jul 28, 2023

[WIP] Upgrade to ember 4.12 #21048

Closed

fix transit test, cleanup

f83d7d2

hashishaw commented Jul 28, 2023

View reviewed changes

Fix MFA test, add TODO comments

2ad3b1e

hashishaw commented Jul 28, 2023

View reviewed changes

ui/app/components/mfa/mfa-login-enforcement-form.js Show resolved Hide resolved

more tidying

bee6570

hashishaw commented Jul 31, 2023

View reviewed changes

hashishaw marked this pull request as ready for review July 31, 2023 14:44

hashishaw and others added 2 commits July 31, 2023 09:45

Add changelog

66bb750

Merge branch 'main' into ui/VAULT-16197/upgrade-ember-4.12

dbf25fb

vercel bot deployed to Preview July 31, 2023 14:51 View deployment

hellobontempo reviewed Jul 31, 2023

View reviewed changes

ui/app/components/mfa/mfa-login-enforcement-form.js Outdated Show resolved Hide resolved

hellobontempo reviewed Jul 31, 2023

View reviewed changes

Use ember-array

ddde559

hellobontempo reviewed Jul 31, 2023

View reviewed changes

ui/tests/helpers/commands.js Outdated

Copy link

Contributor

hellobontempo Jul 31, 2023

Choose a reason for hiding this comment

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

😍

Fix MFA enforcement array with tests

461c71b

zofskeez approved these changes Aug 1, 2023

View reviewed changes

hashishaw added 2 commits August 1, 2023 13:25

fix calling incorrect setOktaNumberChallenge

07e5bfa

Check for store.isDestroying on component willDestroy hooks

a7fef9e

hashishaw merged commit 8731cee into main Aug 1, 2023
99 checks passed

hashishaw deleted the ui/VAULT-16197/upgrade-ember-4.12 branch August 1, 2023 19:02

hellobontempo mentioned this pull request Mar 22, 2024

Cleanup package resolutions #26127

Merged

	return replicationMode ? get(this, replicationMode) : null;
	return replicationMode ? this.replicationMode : null;

UI: Upgrade to Ember 4.12 #22122

UI: Upgrade to Ember 4.12 #22122

Conversation

hashishaw commented Jul 28, 2023 • edited

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

hashishaw Jul 31, 2023 • edited

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

hashishaw Jul 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

hellobontempo Jul 31, 2023 • edited

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

zofskeez left a comment

Choose a reason for hiding this comment

hashishaw commented Jul 28, 2023 •

edited

hashishaw Jul 31, 2023 •

edited

hashishaw Jul 31, 2023 •

edited

hellobontempo Jul 31, 2023 •

edited