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: Upgrade to Ember 4.12 #22122

Merged
merged 28 commits into from Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
09ce409
ember-cli-update ran
hashishaw May 18, 2023
b6812d3
fix incorrect seal status - update clusterModel into class-based
hashishaw May 19, 2023
da76d64
Fix activeCluster not being re-calculated when the store cluster valu…
hashishaw May 19, 2023
09ea562
Prevent unloading after test teardown
hashishaw Jun 2, 2023
082e603
Dont call window.confirm if testing
hashishaw Jun 2, 2023
bf50b35
Prevent test failure from undefined leaderNode
hashishaw Jun 2, 2023
f894d2c
Fix oidc/* test helper - direct access instead of .content
hashishaw Jun 7, 2023
b45d545
secret-edit -- continue transition if model not clean or deleted
hashishaw Jun 20, 2023
bdfb09b
Fix fetchPage test (unloadAll fixed)
hashishaw Jul 11, 2023
92461ca
Fix MFA login enforcement form test
hashishaw Jul 11, 2023
199460a
Fix mount-backend-form test
hashishaw Jul 12, 2023
2826315
stabilize has-permission-test
hashishaw Jul 12, 2023
0fcb370
Ember Upgrade to 4.12 (#21955)
zofskeez Jul 25, 2023
fa692ba
this.owner.lookup is not a promise
hashishaw Jul 26, 2023
4231802
transition to logout as intended even if not authed
hashishaw Jul 26, 2023
29f0d51
Revert "transition to logout as intended even if not authed"
hashishaw Jul 26, 2023
af00582
transition to logout if intended and not authed
hashishaw Jul 26, 2023
e452be2
Add console commands test helper
hashishaw Jul 26, 2023
b19cdce
Fix database tests
hashishaw Jul 28, 2023
f83d7d2
fix transit test, cleanup
hashishaw Jul 28, 2023
2ad3b1e
Fix MFA test, add TODO comments
hashishaw Jul 28, 2023
bee6570
more tidying
hashishaw Jul 28, 2023
66bb750
Add changelog
hashishaw Jul 31, 2023
dbf25fb
Merge branch 'main' into ui/VAULT-16197/upgrade-ember-4.12
hashishaw Jul 31, 2023
ddde559
Use ember-array
hashishaw Jul 31, 2023
461c71b
Fix MFA enforcement array with tests
hashishaw Jul 31, 2023
07e5bfa
fix calling incorrect setOktaNumberChallenge
hashishaw Aug 1, 2023
a7fef9e
Check for store.isDestroying on component willDestroy hooks
hashishaw Aug 1, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ui/.ember-cli
Expand Up @@ -9,8 +9,8 @@
"output-path": "../http/web_ui",

/**
Setting `isTypeScriptProject` to true will force the blueprint generators to generate TypeScript
rather than JavaScript by default, when a TypeScript version of a given blueprint is available.
Setting `isTypeScriptProject` to true will force the blueprint generators to generate TypeScript
rather than JavaScript by default, when a TypeScript version of a given blueprint is available.
*/
"isTypeScriptProject": false
}
18 changes: 7 additions & 11 deletions ui/.eslintrc.js
Expand Up @@ -8,13 +8,14 @@
'use strict';

module.exports = {
parser: 'babel-eslint',
parser: '@babel/eslint-parser',
root: true,
parserOptions: {
ecmaVersion: 2018,
ecmaVersion: 'latest',
sourceType: 'module',
ecmaFeatures: {
legacyDecorators: true,
requireConfigFile: false,
babelOptions: {
plugins: [['@babel/plugin-proposal-decorators', { decoratorsBeforeExport: true }]],
},
},
plugins: ['ember'],
Expand Down Expand Up @@ -45,6 +46,7 @@ module.exports = {
files: [
'./.eslintrc.js',
'./.prettierrc.js',
'./.stylelintrc.js',
'./.template-lintrc.js',
'./ember-cli-build.js',
'./testem.js',
Expand All @@ -60,13 +62,7 @@ module.exports = {
browser: false,
node: true,
},
plugins: ['node'],
extends: ['plugin:node/recommended'],
rules: {
// this can be removed once the following is fixed
// https://github.com/mysticatea/eslint-plugin-node/issues/77
'node/no-unpublished-require': 'off',
},
extends: ['plugin:n/recommended'],
},
{
// test files
Expand Down
6 changes: 6 additions & 0 deletions ui/.prettierrc.js
Expand Up @@ -17,5 +17,11 @@ module.exports = {
printWidth: 125,
},
},
{
files: '*.{js,ts}',
options: {
singleQuote: true,
},
},
],
};
8 changes: 8 additions & 0 deletions ui/.stylelintignore
@@ -0,0 +1,8 @@
# unconventional files
/blueprints/*/files/

# compiled output
/dist/

# addons
/.node_modules.ember-try/
5 changes: 5 additions & 0 deletions ui/.stylelintrc.js
@@ -0,0 +1,5 @@
'use strict';

module.exports = {
extends: ['stylelint-config-standard', 'stylelint-prettier/recommended'],
};
1 change: 1 addition & 0 deletions ui/.template-lintrc.js
Expand Up @@ -46,6 +46,7 @@ module.exports = {
allow: ['supported-auth-backends'],
},
'require-input-label': 'off',
'no-array-prototype-extensions': 'off',
},
ignore: ['lib/story-md', 'tests/**'],
// ember language server vscode extension does not currently respect the ignore field
Expand Down
2 changes: 2 additions & 0 deletions ui/app/components/auth-form.js
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// if possible, it would be more reliable to add a namespace attr to the model so we could filter against the current namespace rather than unloading all
next(() => {
store.unloadAll('auth-method');
});
Expand Down
6 changes: 3 additions & 3 deletions ui/app/components/database-role-edit.js
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Expand Up @@ -62,7 +62,7 @@ export default class DatabaseRoleEdit extends Component {
delete() {
const secret = this.args.model;
const backend = secret.backend;
secret
return secret
.destroyRecord()
.then(() => {
try {
Expand All @@ -89,7 +89,7 @@ export default class DatabaseRoleEdit extends Component {
const path = roleSecret.type === 'static' ? 'static-roles' : 'roles';
roleSecret.set('path', path);
}
roleSecret
return roleSecret
.save()
.then(() => {
try {
Expand All @@ -110,7 +110,7 @@ export default class DatabaseRoleEdit extends Component {
rotateRoleCred(id) {
const backend = this.args.model?.backend;
const adapter = this.store.adapterFor('database/credential');
adapter
return adapter
.rotateRoleCredentials(backend, id)
.then(() => {
this.flashMessages.success(`Success: Credentials for ${id} role were rotated`);
Expand Down
5 changes: 2 additions & 3 deletions ui/app/components/identity/edit-form.js
Expand Up @@ -73,12 +73,11 @@ export default Component.extend({
).drop(),

willDestroy() {
this._super(...arguments);
const model = this.model;
if (!model) return;
if ((model.get('isDirty') && !model.isDestroyed) || !model.isDestroying) {
if (model && model.get('isDirty') && !model.isDestroyed && !model.isDestroying) {
model.rollbackAttributes();
}
this._super(...arguments);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

},

actions: {
Expand Down
2 changes: 2 additions & 0 deletions ui/app/components/mfa/mfa-login-enforcement-form.js
hashishaw marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -65,6 +65,8 @@ export default class MfaLoginEnforcementForm extends Component {
const targetArray = await this.args.model[key];
const targets = targetArray.map((value) => ({ label, key, value }));
this.targets.addObjects(targets);
// eslint-disable-next-line no-self-assign
this.targets = this.targets;
}
}
async resetTargetState() {
Expand Down
15 changes: 10 additions & 5 deletions ui/app/components/mount-backend-form.js
Expand Up @@ -35,12 +35,17 @@ export default class MountBackendForm extends Component {
@tracked errorMessage = '';

willDestroy() {
// if unsaved, we want to unload so it doesn't show up in the auth mount list
super.willDestroy(...arguments);
if (this.args.mountModel) {
const method = this.args.mountModel.isNew ? 'unloadRecord' : 'rollbackAttributes';
this.args.mountModel[method]();
if (this.args?.mountModel) {
try {
// if unsaved, we want to unload so it doesn't show up in the auth mount list
const method = this.args.mountModel.isNew ? 'unloadRecord' : 'rollbackAttributes';
this.args.mountModel[method]();
} catch (e) {
// In component test the store is torn down before willDestroy is called, causing error
console.debug('mountModel rollback failed with error:', e.message); // eslint-disable-line
}
}
super.willDestroy(...arguments);
}

checkPathChange(type) {
Expand Down
4 changes: 2 additions & 2 deletions ui/app/components/role-edit.js
Expand Up @@ -26,10 +26,10 @@ export default Component.extend(FocusOnInsertMixin, {
requestInFlight: or('model.isLoading', 'model.isReloading', 'model.isSaving'),

willDestroyElement() {
this._super(...arguments);
if (this.model && this.model.isError) {
if (this.model && this.model.isError && !this.model.isDestroyed && !this.model.isDestroying) {
this.model.rollbackAttributes();
}
this._super(...arguments);
},

waitForKeyUp: task(function* () {
Expand Down
8 changes: 5 additions & 3 deletions ui/app/components/secret-edit.js
Expand Up @@ -35,14 +35,13 @@ export default class SecretEdit extends Component {
@service store;

@tracked secretData = null;
@tracked isV2 = false;
@tracked codemirrorString = null;

// fired on did-insert from render modifier
@action
createKvData(elem, [model]) {
if (!model.secretData && model.selectedVersion) {
this.isV2 = true;
if (this.isV2) {
// pre-fill secret data from selected version
model.secretData = model.belongsTo('selectedVersion').value().secretData;
}
this.secretData = KVObject.create({ content: [] }).fromJSON(model.secretData);
Expand Down Expand Up @@ -97,6 +96,9 @@ export default class SecretEdit extends Component {
@or('model.isLoading', 'model.isReloading', 'model.isSaving') requestInFlight;
@or('requestInFlight', 'model.isFolder', 'model.flagsIsInvalid') buttonDisabled;

get isV2() {
return !!this.args.model?.selectedVersion;
}
get modelForData() {
const { model } = this.args;
if (!model) return null;
Expand Down
4 changes: 2 additions & 2 deletions ui/app/components/transform-edit-base.js
Expand Up @@ -44,10 +44,10 @@ export default Component.extend(FocusOnInsertMixin, {
},

willDestroyElement() {
this._super(...arguments);
if (this.model && this.model.isError) {
if (this.model && this.model.isError && !this.model.isDestroyed && !this.model.isDestroying) {
this.model.rollbackAttributes();
}
this._super(...arguments);
},

transitionToRoute() {
Expand Down
4 changes: 2 additions & 2 deletions ui/app/components/transit-edit.js
Expand Up @@ -26,10 +26,10 @@ export default Component.extend(FocusOnInsertMixin, {
requestInFlight: or('key.isLoading', 'key.isReloading', 'key.isSaving'),

willDestroyElement() {
this._super(...arguments);
if (this.key && this.key.isError) {
if (this.key && this.key.isError && !this.key.isDestroyed && !this.key.isDestroying) {
this.key.rollbackAttributes();
}
this._super(...arguments);
},

waitForKeyUp: task(function* () {
Expand Down
6 changes: 3 additions & 3 deletions ui/app/initializers/deprecation-filter.js
Expand Up @@ -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')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deprecations are returning 5.0 and 5.0.0

return;
}
return;
next(message, options);
});
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/lib/route-paths.js
Expand Up @@ -6,6 +6,7 @@
export const INIT = 'vault.cluster.init';
export const UNSEAL = 'vault.cluster.unseal';
export const AUTH = 'vault.cluster.auth';
export const LOGOUT = 'vault.cluster.logout';
export const REDIRECT = 'vault.cluster.redirect';
export const CLUSTER = 'vault.cluster';
export const CLUSTER_INDEX = 'vault.cluster.index';
Expand Down
17 changes: 17 additions & 0 deletions ui/app/macros/maybe-query-record.js
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* see this GH issue for information on the fix https://github.com/emberjs/ember.js/issues/16541#issuecomment-382403523
*/
ObjectProxy.reopen({
unknownProperty(key) {
if (this.isDestroying || this.isDestroyed) {
return;
}

if (this.content && (this.content.isDestroying || this.content.isDestroyed)) {
return;
}

return this._super(key);
},
});

export function maybeQueryRecord(modelName, options = {}, ...keys) {
return computed(...keys, 'store', {
Expand Down
5 changes: 5 additions & 0 deletions ui/app/mixins/model-boundary-route.js
Expand Up @@ -56,6 +56,11 @@ export default Mixin.create({
);
return;
}
if (this.store.isDestroyed || this.store.isDestroying) {
// Prevent unload attempt after test teardown, resulting in test failure
return;
}

if (modelType) {
this.store.unloadAll(modelType);
}
Expand Down
1 change: 0 additions & 1 deletion ui/app/mixins/unload-model-route.js
Expand Up @@ -19,7 +19,6 @@ export default Mixin.create({
return;
}
removeRecord(this.store, model);
model.destroy();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't seem to be needed and removing it doesn't seem to change anything other than get rid of some errors in tests. I believe that calling unloadRecord is sufficient and ember data should take care of the rest.

// it's important to unset the model on the controller since controllers are singletons
this.controller.set(modelPath, null);
},
Expand Down
2 changes: 2 additions & 0 deletions ui/app/mixins/unsaved-model-route.js
Expand Up @@ -4,6 +4,7 @@
*/

import Mixin from '@ember/object/mixin';
import Ember from 'ember';

// this mixin relies on `unload-model-route` also being used
export default Mixin.create({
Expand All @@ -15,6 +16,7 @@ export default Mixin.create({
}
if (model.hasDirtyAttributes) {
if (
Ember.testing ||
window.confirm(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

window.confirm blows up in testing

'You have unsaved changes. Navigating away will discard these changes. Are you sure you want to discard your changes?'
)
Expand Down