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 - ent init #5428

Merged
merged 9 commits into from Sep 28, 2018
Merged

UI - ent init #5428

merged 9 commits into from Sep 28, 2018

Conversation

meirish
Copy link
Contributor

@meirish meirish commented Sep 27, 2018

Though the UI started out at enterprise-only, it never had the ability to init any seals that weren't shamir-backed (the default if the seal stanza is absent from the vault config). This PR enables initializing a Vault cluster using any of the documented seal types.

The only visual difference is after you init, the message up top is a bit different because non-shamir seals will auto-unseal, and we now progress the user to Authenticate directly after init.

screen shot 2018-09-26 at 9 31 05 pm

NOTE: This PR is dependent on the API change in #5424 to function properly - we need to know the seal type in order send the correct init parameters.

@meirish meirish requested review from a team September 27, 2018 20:46
Copy link
Contributor

@joshuaogle joshuaogle left a comment

Choose a reason for hiding this comment

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

I'll let someone else review the Javascript but everything looks good to me. The new copy sounds great 👍

</h1>
{{#with (or keyData.recovery_keys keyData.keys) as |keyArray|}}
<h1 class="title is-4">
Vault has been initialized! {{#if (eq keyArray.length 1)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any harm in moving the conditional to the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! I'lld do that

Copy link

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I just had some inconsequential things to say.

@@ -13,6 +13,7 @@ const DEFAULTS = {

export default Controller.extend(DEFAULTS, {
wizard: service(),
model: null,

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol no, habit from components

@@ -1,4 +1,3 @@
import { computed } from '@ember/object';
import { alias, and, equal } from '@ember/object/computed';
import DS from 'ember-data';
const { attr } = DS;

Choose a reason for hiding this comment

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

Unrelated, but you ought to be able to use modules for Ember Data as well now.

import Model from 'ember-data/model';
import attr from 'ember-data/attr';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh I guess the codemod doesn't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oddly the guides still show the old way - gonna hold off on this now and do it all at once in a different PR.

Here are your {{pluralize keyData.keys.length "key"}}.
{{/if}}
</h1>
{{#with (or keyData.recovery_keys keyData.keys) as |keyArray|}}

Choose a reason for hiding this comment

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

I believe with is deprecated in favor of let now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo and it can yield multiple items 😙👌

@stringify={{true}}
>
<DownloadButton @data={{keyData}} @filename={{keyFilename}} @mime="application/json" @extension="json" @class="button is-ghost"
@stringify={{true}}>

Choose a reason for hiding this comment

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

This is some weird formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh yes js-beautify's default which VSCode uses does "break attrs only after max line length" by default - fixed it to break all attrs which is also kinda weird, but more easily read imo.

@@ -157,4 +149,4 @@
</form>
</Page.content>
{{/if}}
</SplashPage>
</SplashPage>

Choose a reason for hiding this comment

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

Intentional lack of newline or fallout from switching to VSCode 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱 also another VSCode thing which I think I've fixed...


test('cloud seal init', async function(assert) {
setInitResponse(this.server, CLOUD_SEAL_RESPONSE);
setStatusResponse(this.server, CLOUD_SEAL_STATUS_RESPONSE);

Choose a reason for hiding this comment

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

I like these helpers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! maybe overkill for 2 tests, but felt like it cleaned things up nicely

@@ -34,7 +34,10 @@ export default {
onEntry: { type: 'render', level: 'feature', component: 'wizard/init-setup' },
},
save: {
on: { TOUNSEAL: 'unseal' },
on: {
TOUNSEAL: 'unseal',

Choose a reason for hiding this comment

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

These look slightly weird, like they should have an _ in them, is this due to some sort of restriction, or just then way you've gone? (wheres a 🦭 🌊 emoji when you need one 😂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah this was more keeping in line with what's there already - I'll make a note to add underscores later as we've got more work to do on the on boarding stuff.

@johncowen
Copy link

Should these use one of the 'sensitive info toggle` components we have? Especially seeing as I might just want to download them without displaying them.

@meirish
Copy link
Contributor Author

meirish commented Sep 28, 2018

@johncowen re: sensitive input - yep! but it needs to be reworked a bit. I actually tried adding it but there were some layout issues that weren't immediately obvious how to fix. We're going to be adding that in a lot more places for the next major release though.

@meirish meirish added this to the 0.11.2 milestone Sep 28, 2018
@meirish meirish merged commit d438d2f into master Sep 28, 2018
@meirish meirish deleted the ui-ent-init branch September 28, 2018 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants