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

PKI Redesign: setup Ember Engine #16925

Merged
merged 12 commits into from
Sep 2, 2022
Merged

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Aug 29, 2022

This PR is the first of the PKI redesign project. We will be pushing our changes to main, but keeping the work hidden behind an Ember engine (later exposed in the 1.13 release). The mount for this engine will be commented out on the router.js file so a user can't accidentally fall upon the new routing. Thus, you should never see a pki mount added to the router.js file until the project is ready.

image

Notes:

  1. The new engine—similar to KMIP—can be reached when added as a mount in the router.js file as http://localhost:4200/ui/vault/secrets/pki-1/pki/<route> where pki-1 is the secret engine mount's name.

Compare this to the current route (which doesn't have the appended pki).
http://localhost:4200/ui/vault/secrets/pki-1/<route>

  1. Ran test locally, twice in a row, including Enterprise. Everything looked good.

  2. If you want to test on the ui, be sure to yarn install before hand (And you'll need to yarn install when you move off the branch too).

@Monkeychip Monkeychip marked this pull request as ready for review August 30, 2022 16:08
@Monkeychip Monkeychip changed the title PKI Redesign: setup Ember Engine and Overview Landing page PKI Redesign: setup Ember Engine Sep 1, 2022

const { modulePrefix } = config;
/* eslint-disable ember/avoid-leaking-state-in-ember-objects */
const Eng = Engine.extend({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update this syntax to extends Engine - that way we can also remove the eslint-disable comment?

Sample below from ember engines quickstart which is also what Chelsea's used in the Vault Insights engine PR

// addon/engine.js
import Engine from '@ember/engine';

import loadInitializers from 'ember-load-initializers';
import Resolver from 'ember-resolver';

import config from './config/environment';

const { modulePrefix } = config;

export default class PkiEngine extends Engine {
  modulePrefix = modulePrefix;
  Resolver = Resolver;
}

loadInitializers(PkiEngine, modulePrefix);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! I'll update.

@@ -0,0 +1,3 @@
import Resolver from 'ember-resolver';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this file if wherever we have import Resolver from './resolver'; we change to import Resolver from 'ember-resolver';

/* eslint-disable node/no-extraneous-require */
'use strict';

const EngineAddon = require('ember-engines/lib/engine-addon');
Copy link
Contributor

Choose a reason for hiding this comment

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

Also taken from the quickstart guide, but could we do something like this instead?

const { buildEngine } = require('ember-engines/lib/engine-addon');

module.exports = buildEngine({
  name: 'pki',
  lazyLoading: {
    enabled: false
  }
});

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

Just some comments! As I haven't worked much with engines I'm not confident enough in to give a full approval

ui/app/router.js Outdated
@@ -114,6 +114,7 @@ Router.map(function () {
this.route('backends', { path: '/' });
this.route('backend', { path: '/:backend' }, function () {
this.mount('kmip');
// this.mount('pki'); // ARG TODO always comment out when pushing to main
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought but if you used the environment from the config (which is already imported in the router file) you could conditionally use the mount for development and not have to worry about commenting it out. Something like this:

if (config.environment !== 'production') {
  this.mount('pki');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Amended, and tested.

@zofskeez
Copy link
Contributor

zofskeez commented Sep 1, 2022

I have a question about the routing. If the engine is pki would it make more sense for that to come before the dyanmic engine name?
http://localhost:4200/ui/vault/secrets/pki/<engine-name>/<route>

Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Looks like you're off to a good start. Thanks!

@Monkeychip Monkeychip merged commit e601329 into main Sep 2, 2022
@Monkeychip Monkeychip deleted the ui/VAULT-7963/ember-engine-pki-create branch September 2, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants