Create/Edit source secret #150

jhadvig · 2018-06-21T17:21:12Z

Adding subform for creating source secret types:

kubernetes.io/basic-auth
kubernetes.io/ssh-auth

Screens:

@openshift/team-ux-review PTAL

Going to do additional code refactoring

spadgett · 2018-06-21T17:31:37Z

We might call it "Git Source Secret" to make it more obvious what it's used for.

serenamarie125

the placement of the dropdown menu doesn't look correct - @beanh66 did you have a mock for this

benjaminapetersen

Just some small comments!

benjaminapetersen · 2018-06-21T17:36:00Z

frontend/public/components/secrets/create-secret.tsx

+}
+
+const determineDefaultSecretType = (typeAbstraction) => {
+  if (typeAbstraction === 'source'){


Just a style consistency nit:

return typeAbstraction === 'source' ? SecretType.basicAut : SecretType.opaque;

benjaminapetersen · 2018-06-21T17:40:11Z

frontend/public/components/secrets/create-secret.tsx

    const { saveButtonText } = this.props;

-    const explanation = 'Webhook secrets allow you to authenticate a webhook trigger.';
-    const subform = <WebHookSecretSubform onChange={this.onDataChanged.bind(this)} stringData={this.state.stringData} />;
+    let explanation = null;


If you can const and separate the logic on these, I would:

const isWebhook = this.state.secretTypeAbstraction === 'webhook'; const explanation = isWebhook ? 'Webhook secrets allow you to authenticate a webhook trigger.' : 'Source secrets allow you to authenticate against the SCM server.'; const subform = isWebhook ? <WebHookSecretSubform onChange={this.onDataChanged.bind(this)} stringData={this.state.stringData} /> : <SourceSecretSubform onChange={this.onDataChanged.bind(this)} stringData={this.state.stringData} />;

There are going to be a bunch more subforms, so this won't scale. We'll eventually have a switch statement for several types.

Thats fine. A switch or an object lookup works for me.

benjaminapetersen · 2018-06-21T17:43:22Z

frontend/public/components/secrets/create-secret.tsx

    }, () => this.props.onChange(this.state));
  }
  render () {
    return <div className="form-group">
      <label className="control-label" htmlFor="webhook-secret-key">Webhook Secret Key</label>
      <div className="input-group">
-        <input className="form-control" id="webhook-secret-key" type="text" name="webhookSecretKey" onChange={this.changeWebHookSecretkey} value={this.state.WebHookSecretKey} required/>
+        <input className="form-control" id="webhook-secret-key" type="text" name="webhookSecretKey" onChange={this.changeWebHookSecretkey} value={this.state.stringData.WebHookSecretKey} required/>


Do we need to worry about ensuring id attribs are always unique, with something like _.uniqueId('webhook-secret-key-')? We ran into this error a lot in the old console, where a component initially only appeared on the page once... then later would get reused & we would have id conflicts. I'd suggest we just always ensure its unique.

I wouldn't worry about it here. Presumably we won't show the form more than once on the page

benjaminapetersen · 2018-06-21T17:46:04Z

frontend/public/components/secrets/create-secret.tsx

+    }, () => this.props.onChange(this.state));
+  }
+  render () {
+    const basicAuthSubform = <React.Fragment>


Do these need to live inside render? I'd prob rather favor smaller components & just pull them out as separate functions:

const basicAuthSubform: React.SFC = ({props}) => <React.Fragment> ... </React.Fragment>

I hesitate to risk render bloating over time.

benjaminapetersen · 2018-06-21T17:46:34Z

frontend/public/components/secrets/create-secret.tsx

+          <input type="text" className="form-control" readOnly disabled/>
+          <span className="input-group-btn">
+            <span className="btn btn-default btn-file">
+              Browse&hellip;


Is Browse… correct?

It should be -> https://github.com/openshift/origin-web-console/blob/efea73189b2e79f8923279b0935374c47d760942/app/views/directives/osc-file-input.html#L16

benjaminapetersen · 2018-06-21T17:48:12Z

frontend/public/components/secrets/create-secret.tsx

+          </select>
+        </div>
+      </div>
+      { this.state.authenticationType === 'kubernetes.io/basic-auth' ? basicAuthSubform : sshAuthSubform }


Per above comment, then:

{ this.state.authenticationType === 'kubernetes.io/basic-auth' ? <BasicAuthSubform {...theseProps} /> : <SSHAuthSubform {...otherProps}> }

jhadvig · 2018-06-21T18:26:38Z

@benjaminapetersen thanks ! The PR is in a early stage, doing the refact ATM, will try to use the HOC for this. Wanted to have a "working version" asap so the UX folks can take a look on it.

beanh66 · 2018-06-21T19:25:44Z

@serenamarie125 Yes, agreed the dropdown is not aligned correctly, but I think that is a known issue and @jhadvig was addressing in another PR. Correct me if I'm wrong @jhadvig!

jhadvig · 2018-06-22T12:21:07Z

Yeah its a known issue, I've described it here. Will open an issue for that, so we can track it.
@serenamarie125 @beanh66 any other suggestions ? Thanks ! :)

serenamarie125

LGTM! Thanks for the clarifications

robszumski · 2018-06-22T17:41:36Z

Jenkins push

jhadvig · 2018-06-25T11:09:59Z

@spadgett @benjaminapetersen I've refactored and updated the PR and the create-secret.tsx file.

Refactoring includes:

Properly naming class and function componnets
Adding a Higher Order Component - withSecretForm - which is responsible or constructing different secret forms (source, webhook) and returns SecretFormComponent that represents particular form

The overall component hierarchy looks like:

SecretFormComponent --  WebHookSecretForm
                    \
                     \  SourceSecretForm  --  BasicAuthSubform
                                          \
                                           \  SSHAuthSubform  -- FileInput

Also added:

FileInput component that is responsible for uploading files

Had created support for drag'n'drop, but since this PR already has a lot of changes, so I will submit the PR after this is merged.

Had done a lot of manual testing where I was creating and then editing the secrets.

The UX design stayed without any changes.

PTAL

beanh66

LGTM!

spadgett

Thanks @jhadvig. I think the HOC works well

Some comments

spadgett · 2018-06-25T15:00:11Z

frontend/public/components/_secret.scss

@@ -0,0 +1,29 @@
+.btn-file {


It's not clear to me why we need special styles for the button if we're already using Bootstrap .input-group-btn?

So in case we want the input field with Browse btn for picking file, we need additional styling since the Bootstrap does not offer this styling by default.
Check origin-web-console styling and usage. Also was browsing similar use-cases and more less this approach is pretty common.

spadgett · 2018-06-25T15:02:28Z

frontend/public/components/secrets/create-secret.tsx

+export enum SecretType {
+  basicAuth = 'kubernetes.io/basic-auth',
+  sshAuth = 'kubernetes.io/ssh-auth',
+  opaque = 'Opaque'


Consider reusing values from the enum when we define the secret list filters

spadgett · 2018-06-25T15:03:59Z

frontend/public/components/secrets/create-secret.tsx

 }

+const secretFormExplanation = {
+  source: 'Source secrets allow you to authenticate against the SCM server.',


Does this work?

const secretFormExplanation = { [SecretTypeAbstraction.source]: 'Source secrets allow you to authenticate against the SCM server.', [SecretTypeAbstraction.webhook]: 'Webhook secrets allow you to authenticate a webhook trigger.', };

spadgett · 2018-06-25T15:05:13Z

frontend/public/components/secrets/create-secret.tsx

+
+const determineDefaultSecretType = (typeAbstraction) => {
+  return typeAbstraction === 'source' ? SecretType.basicAuth : SecretType.opaque;
+};


const determineDefaultSecretType = (typeAbstraction: SecretTypeAbstraction) => { return typeAbstraction === SecretTypeAbstraction.source ? SecretType.basicAuth : SecretType.opaque; };

spadgett · 2018-06-25T15:08:39Z

frontend/public/components/secrets/create-secret.tsx

-    const existingObj = _.pick(props.obj, ['metadata', 'type']);
-    const existingData = _.get(props.obj, 'data');
-    const secret = _.defaultsDeep({}, props.fixed, existingObj, {
+    const inputObj = _.get(props.obj, 'data');


props.obj.data is the secret object and not just the secret data? That's confusing :/

So there is going to be a props.obj.data.data property? Usually when you repeat property names like that, something isn't named well.

spadgett · 2018-06-25T16:35:13Z

frontend/public/components/secrets/create-secret.tsx

+    return <div className="form-group">
+      <label className="control-label" htmlFor="ssh-privatekey">SSH Private Key</label>
+      <div className="modal-body__field">
+        <FileInput onChange={this.onFileChange.bind(this)}/>


Prefer to bind in constructor.

nit: space before / in self-closing tags since that's the style used throughout the existing code

spadgett · 2018-06-25T16:35:57Z

frontend/public/components/secrets/create-secret.tsx

+          name="privateKey"
+          onChange={this.changeData}
+          value={this.state['ssh-privatekey']}
+          required>


nit: self-closing tag?

aria-describedby would be good here

spadgett · 2018-06-25T16:48:54Z

frontend/public/components/secrets/create-secret.tsx

  onChange: Function;
-  stringData: {[WebHookSecretKey: string]: string};
+  stringData: {[WebHookSecretKey: string]: string},


Missed this only previous review, but this should be

stringData: { WebHookSecretKey: string, }

spadgett · 2018-06-25T16:50:38Z

frontend/public/components/utils/file-input.tsx

@@ -0,0 +1,50 @@
+import * as React from 'react';
+
+export class FileInput extends React.Component<fileInputProps, fileInputState> {


👍 nice to see this added with so little code

yeah, even the drag&Drop is pretty small 👍

spadgett · 2018-06-25T16:52:10Z

frontend/public/components/secrets/create-secret.tsx

+};
+
+// withSecretForm returns SubformComponent which is a Higher Order Component for all the types of secret forms.
+const withSecretForm = (SubformComponent) => class SecretFormComponent extends React.Component<BaseEditSecretProps_, BaseEditSecretState_> {


I think you can take "Component" out of the name. I'd call this class SecretForm

Make even more sense ! :)

spadgett · 2018-06-25T19:48:28Z

@jhadvig This PR is big enough that it would be good to address feedback in separate commits, then we can squash before merge. Then it's easier to review just what's been updated.

jhadvig · 2018-06-26T10:22:02Z

@spadgett comments addressed in a separate commit and also commented back on two of the comments:

PTAL

spadgett

Thanks @jhadvig. Still looking, but here are a few comments on the CSS

spadgett · 2018-06-26T11:31:56Z

frontend/public/components/_secret.scss

@@ -0,0 +1,31 @@
+.co-create-secret-form {
+  .btn-file {


The file picker is generic and will eventually be used outside the secret form. We shouldn't scope the styles to the secret form.

Please use co- prefix for new styles.

spadgett · 2018-06-26T11:33:21Z

frontend/public/components/_secret.scss

@@ -0,0 +1,31 @@
+.co-create-secret-form {


BEM discourages nesting, and we should give the classes appropriate names for BEM.

http://getbem.com/

Style properties should be alphabetized.

Saw nesting in our code-base so thought it's ok, but will try to avoid it ! Thanks :)

spadgett · 2018-06-26T11:34:59Z

frontend/public/components/_secret.scss

+  }
+
+  .help-block {
+    margin-bottom: 10px;


Why do we need to override the default margin for help-block?

jhadvig · 2018-06-26T12:37:36Z

@spadgett comments addressed. Thanks and PTAL :)

spadgett

LGTM. Please squash.

Thanks @jhadvig !

spadgett · 2018-06-26T13:43:52Z

frontend/public/components/utils/_file-input.scss

+    outline: none;
+    background: white;
+    cursor: inherit;
+    display: block;


nit: alphabetize

jhadvig · 2018-06-26T14:02:29Z

@spadgett comment addressed and squashed. Lets merge this baby 👍

spadgett

Nice work

/lgtm

Comments addressed

spadgett · 2018-06-26T16:41:06Z

/refresh

stevekuznetsov · 2018-06-26T17:07:08Z

/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2018

serenamarie125 reviewed Jun 21, 2018

View reviewed changes

jhadvig force-pushed the source-subform branch from a42f5a4 to a8b674e Compare June 21, 2018 17:38

benjaminapetersen previously requested changes Jun 21, 2018

View reviewed changes

serenamarie125 approved these changes Jun 22, 2018

View reviewed changes

jhadvig force-pushed the source-subform branch from a8b674e to 0a265d6 Compare June 25, 2018 10:52

jhadvig changed the title ~~[WIP] Create/Edit source secret~~ Create/Edit source secret Jun 25, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2018

beanh66 reviewed Jun 25, 2018

View reviewed changes

spadgett requested changes Jun 25, 2018

View reviewed changes

spadgett reviewed Jun 26, 2018

View reviewed changes

jhadvig force-pushed the source-subform branch from 763f853 to b795dd6 Compare June 26, 2018 14:00

create/edit source secret

20d555d

jhadvig force-pushed the source-subform branch from b795dd6 to 20d555d Compare June 26, 2018 14:13

spadgett approved these changes Jun 26, 2018

View reviewed changes

openshift-ci-robot assigned spadgett Jun 26, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018

openshift-merge-robot merged commit c79042d into openshift:master Jun 26, 2018

		@@ -0,0 +1,50 @@
		import * as React from 'react';

		export class FileInput extends React.Component<fileInputProps, fileInputState> {

Create/Edit source secret #150

Create/Edit source secret #150

Uh oh!

Conversation

jhadvig commented Jun 21, 2018

Uh oh!

spadgett commented Jun 21, 2018

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhadvig commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beanh66 commented Jun 21, 2018

Uh oh!

jhadvig commented Jun 22, 2018

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

robszumski commented Jun 22, 2018

Uh oh!

jhadvig commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beanh66 left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jhadvig commented Jun 21, 2018 •

edited

Loading

jhadvig commented Jun 25, 2018 •

edited

Loading