Create/Edit page for Webhook secrets #94

jhadvig · 2018-06-05T16:01:21Z

Added button with Create New dropdown button, which (for now, rest will be added in a follow up) contains:

Create Webhook Secret
Create via YAML

When creating the secret Im adding all the values to the stringData which will take care of encoding the data.

I've also added following actions for individual secrets:

Modify Labels
Modify Annotations
Duplicate Secret
Edit Secret
Delete Secret

PTAL

spadgett · 2018-06-05T16:24:04Z

frontend/package.json

@@ -68,6 +68,7 @@
    "plotly.js": "1.28.x",
    "prop-types": "15.6.x",
    "react": "16.x",
+    "react-ace": "6.1.x",


We're already using Ace editor elsewhere (edit YAML). I think we should be able to use Ace here without the new dependency. Can you check?

https://github.com/openshift/console/blob/master/frontend/public/components/edit-yaml.jsx

Sorry I see you comment on this in the description. I think we should discuss further. How large is the library?

from what I see in the node_modules it's 2MB. Just for comparision the brace has 8.5MB

Don't look at node_modules size. Use yarn run analyze to see the size of the built js.

react-ace is a thin wrapper around brace:

https://github.com/securingsincity/react-ace/blob/master/package.json#L63:

"dependencies": { "brace": "^0.11.0", ... },

https://github.com/securingsincity/react-ace/blob/master/src/ace.js#L1:

import ace from 'brace'

react-ace itself is 46.47 KB. Previously we were async loading brace only when needed. Since secret.jsx now imports brace (747KB), our main bundle of vendored deps has increased in size by ~40%.

Are there any interactions here that aren't 90% copy/paste - ie, why do we need a text editor at all? If you do need one, you should use AsyncComponent to load it only when needed since the vast majority of users will not create a secret.

spadgett · 2018-06-05T16:42:20Z

cc @tlwu2013

robszumski · 2018-06-06T18:02:49Z

Throwing out this multi-create button pattern that we have for OLM:

What does @openshift/team-ux-review think of offloading the selection type to this button, which will reduce one of the fields on the screen, plus give you an option to jump to a raw YAML editor. Using a dropdown to switch this will alter the page a ton, plus you have no way to get back.

Create Source Secret...
Create Key/Value Secret...
Create Image Secret...
Create Webhook Secret...
Create via YAML...

Also, I think we need some microcopy work here and none of those phrases mean anything to me:

"source" and "image" are very generic and don't match a Kubernetes-ism that I may be looking for
I renamed "generic" -> "key/value" above
Should "Image" be "ImagePullSecret" (direct) or "Pull Secret" (friendlier)

kans · 2018-06-06T21:32:17Z

Do you mean Opaque instead of Generic? If so, lets use the k8s term.

spadgett · 2018-06-07T15:25:37Z

Not necessarily against using Opaque, but kubectl create secret does call it "generic".

spadgett · 2018-06-07T19:29:22Z

@jhadvig I'd suggest doing these one at a time in separate PRs, maybe starting with opaque secrets. I think we'll be able to get these reviewed and merged quicker that way.

I don't know if the name/value editor will work as-is for generic secrets since they often have multiline values (e.g. certificates and keys).

It would be good to structure this in such a way that we can use the same components for editing secrets, too.

cshinn · 2018-06-08T15:29:52Z

I think @robszumski's idea makes sense. Especially if that selection will drastically change the form underneath. As for terminology, sticking with the K8s standards is probably a good idea

jhadvig · 2018-06-13T17:56:29Z

@spadgett I've update the PR based on the comments from @robszumski
I've replaced the Create Secret button with Create New dropdown button, which (for now, rest will be added in a follow up) contains:

Create Webhook Secret
Create via YAML

When creating the secret Im adding all the values to the stringData which will take care of encoding the data.

I've also added following actions for individual secrets:

Modify Labels
Modify Annotations
Duplicate Secret
Edit Secret
Delete Secret

PTAL

cshinn · 2018-06-13T18:15:42Z

@jhadvig the dropdown panel on the "Create New" button looks a bit misaligned. Looks great other than that

beanh66 · 2018-06-13T20:35:32Z

@jhadvig Thanks this looks good, I just have a few comments:

Can we fix the alignment as Chris mentioned?
The dropdown can just say "Create" with the caret
The options can then be "Secret from YAML" and "Webhook Secret"
Options should be alphabetical
On the webhook secret form, the padding on the right should match the padding on the left if possible and the action button should be "Create" instead of "Create Secret"

alecmerdler

We don't need to add complexity to list-page.jsx when the use case is already handled. Also, you should update the description to accurately reflect the state of the PR.

alecmerdler · 2018-06-14T00:59:23Z

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

+    case 'kubernetes.io/dockerconfigjson':
+    case 'kubernetes.io/dockercfg':
+      determinedType = 'image';
+      break;


Just return after each case.

alecmerdler · 2018-06-14T01:09:16Z

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

+        case 'source':
+        case 'image':
+        case 'generic':
+        case 'webhook':


Are these all supposed to evaluate to WebHookSecretSubform?

No.. those are just subform options that console will eventually exist

Right now the case statements above will fall through to the 'webhook' option. I would suggest YAGNI here and remove the switch block altogether.

True, but the switch will be there once we add all the subforms options. Just wanted to have all the options on mind. But don't have any strong preferences regarding changing it to if statement :)

I'd rather leave out things we don't support yet in this PR.

alecmerdler · 2018-06-14T01:12:12Z

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

+
+const BindingLoadingWrapper = props => {
+  const fixed = {};
+  _.each(props.fixedKeys, k => fixed[k] = _.get(props.obj.data, k));


const fixed = _.reduce(props.fixedKeys, (acc, k) => ({...acc, k: _.get(props.obj.data, k)}), {});

alecmerdler · 2018-06-14T01:19:45Z

frontend/public/components/factory/list-page.jsx

@@ -246,7 +246,19 @@ export const ListPage = props => {
      href = namespaced ? `/k8s/ns/${namespace || 'default'}/${ref}/new` : `/k8s/cluster/${ref}/new`;
    } catch (unused) { /**/ }
  }
-  const createProps = createHandler ? {onClick: createHandler} : {to: href};
+  let createProps;


This isn't necessary, it is already handled in the FireMan_ component.

Not sure how the FireMan_ will evaluate the truly the Dropdown component, since the ListPage component sets createProps either with onClick or to keys, there is no items key set, so there is no way Dropdown will be rendered. Or maybe I'm missing something

See the "Create New" button for the ClusterServiceVersion-v1 detail view and its implementation.

But the implementation counts with setting the items which is missing clearly in the ListPage component, since only onClick and to keys are set and therefor there is no way to create a Dropdown button. From what I can tell, those two implementations are used for different use-cases

I see what you mean. Try this:

Allow passing createProps as a prop to ListPage

Change this line to:

const createProps = createProps || (createHandler ? {onClick: createHandler} : {to: href});

Change SecretsPage to this:

const SecretsPage = props => { const createItems = { // source: 'Create Source Secret', // image: 'Create Image Pull Secret', // generic: 'Create Key/Value Secret', webhook: 'Webhook Secret', yaml: 'Secret from YAML', }; const createProps = { items: createItems, createLink: (type) => `/k8s/ns/${props.obj.metadata.namespace}/secrets/${type === 'yaml' ? 'new' : type}` }; return <ListPage ListComponent={SecretsList} canCreate={true} rowFilters={filters} createButtonText="Create" createProps={createProps} {...props} />; };

I tried this locally and it works.

jhadvig · 2018-06-14T09:18:13Z

@beanh66

Options should be alphabetical

I understand the point but thought that options that will render an actual form should be first and the YAML editor should be the last one and by that somehow logically group them.
Thoughts ?

On the webhook secret form, the padding on the right should match the padding on the left if possible

Sorry for the misleading screen, not sure why but when I took it it chopped off some pixeles from the left padding. The padding is the same on both sides - 30px

Regarding the issue @cshinn mentioned in his comment, this is an issue in the Dropdown component (it's also visible in #94 (comment)), which would require rework the component itself, so I would rather do it as a followup and open an issue for that one, if you don't mind.

beanh66 · 2018-06-15T12:37:10Z

@jhadvig Got it, that makes sense to me on the order.

spadgett · 2018-06-13T21:08:45Z

frontend/public/components/app.jsx

@@ -175,6 +176,13 @@ class App extends React.PureComponent {
          <Route path="/k8s/ns/:ns/roles/:name/:rule/edit" exact component={EditRulePage} />
          <Route path="/k8s/ns/:ns/roles" exact component={rolesListPage} />

+          <Route path="/k8s/cluster/secrets/new/:type" exact component={props => <CreateSecret {...props} kind="Secret" />} />


Can secrets be cluster scoped? I thought they had to be in a namespace

spadgett · 2018-06-13T21:10:24Z

frontend/public/components/factory/list-page.jsx

+    createProps = {
+      items: createHandler,
+      createLink(param) {
+        return param === 'yaml' ? href : `${href}/${param}`;


I'd call encodeURIComponent on param to be safe

spadgett · 2018-06-13T21:14:46Z

frontend/public/components/secret.scss

@@ -0,0 +1,4 @@
+.separator {


This class doesn't look specific to secrets. It should probably go in core. Suggest adding the .co- prefix, too.

Honestly, I would just remove the separator, though. I think it's better without it.

spadgett · 2018-06-13T21:15:57Z

frontend/public/components/secret.jsx

+  // source: 'Create Source Secret',
+  // image: 'Create Image Pull Secret',
+  // generic: 'Create Key/Value Secret',
+  webhook: 'Create Webhook Secret',


We only want to show the webhook secret option if this is an OpenShift cluster. You want connectToFlags(FLAGS.OPENSHIFT) to check

spadgett · 2018-06-13T21:17:57Z

frontend/public/components/secret.jsx

+  Cog.factory.ModifyLabels,
+  Cog.factory.ModifyAnnotations,
+  (kind, obj) => ({
+    label: `Duplicate ${kind.label}...`,


Interesting, is there a specific use case for copy that you had in mind?

I've seen that some of the sereources have the option to duplicate, but to be honest I wasnt sure if it should be available for secrets. Will put it away

spadgett · 2018-06-15T19:01:33Z

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

+      stringData = secretsData;
+      this.setState({stringData});
+    }
+    changeSecretName (event) {


I would suggest names onDataChanged and onNameChanged

spadgett · 2018-06-15T19:14:19Z

frontend/public/components/secrets/subforms.jsx

+    return Math.floor((1 + Math.random()) * 0x10000)
+      .toString(16)
+      .substring(1);
+  }


const s4 = () => Math.floor((1 + Math.random()) * 0x10000).toString(16).substring(1);

spadgett · 2018-06-15T19:14:50Z

frontend/public/components/secrets/subforms.jsx

+import * as React from 'react';
+
+const generateSecret = () => {
+  //http://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript


super nit here, but can you put a space before the http in the comment :)

spadgett · 2018-06-15T19:17:32Z

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

+          <fieldset disabled={!this.props.isCreate}>
+            <div className="form-group">
+              <label className="control-label">Secret Name</label>
+              <div className="modal-body__field">


Why do the classes have modal-body in them when they're not used in a modal?

spadgett · 2018-06-15T19:19:53Z

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

+      e.preventDefault();
+      const { kind, metadata } = this.state.secret;
+      this.setState({ inProgress: true });
+      const newSecretObject = _.assign(this.state.secret, {stringData: this.state.stringData});


This mutates the original object, so is pretty much the same as

this.state.secret.stringData = this.state.stringData;

But I'd suggest

const { stringData } = this.state.stringData; const newSecret = _.assign({}, this.state.secret, { stringData });

spadgett · 2018-06-15T19:31:39Z

It looks like we need to update the integration tests for this change.

jhadvig · 2018-06-18T17:00:26Z

@spadgett I've updated the PR:

rewrote the subforms and create-secret into typescript - had to update button-bar.jsx because of that
updated the intergration-tests
only when creating webhook secret the form will be displayed, otherwise the YAML editor. The same applies for editing secrets, there only WebHookSecretKey key has to be present in the data.

Although I'm not sure why the tests are failing :-/

spadgett · 2018-06-18T17:18:00Z

Ignore the ci/prow/* statuses.

Linting is failing, though:

go/src/github.com/openshift/console/frontend/public/components/utils/dropdown.jsx
  error Unexpected string concatenation prefer-template

spadgett · 2018-06-18T17:18:59Z

You can always use yarn run lint to run the linter locally

jhadvig · 2018-06-19T12:37:05Z

@spadgett sorry for that ... thought that interpolating id="${itemKey}-link" wont cause lint to scream, but apparently id={${itemKey}-link} is the right form

spadgett · 2018-06-19T16:10:40Z

frontend/integration-tests/tests/crud.scenario.ts

@@ -85,7 +85,12 @@ describe('Kubernetes resource CRUD operations', () => {
      }

      it('displays a YAML editor for creating a new resource instance', async() => {
-        await crudView.createYAMLButton.click();
+        if (kind === 'Secret') {


Instead of checking kind, can we check for the existence of #item-create? Then if we add similar buttons to other pages, the tests don't need to change.

spadgett · 2018-06-19T16:15:07Z

frontend/public/components/secret.jsx

@@ -16,7 +16,22 @@ data:
  username: YWRtaW4=
  password: MWYyZDFlMmU2N2Rm`);

-const menuActions = Cog.factory.common;
+const editInYaml = obj => {
+  if (obj.type === 'Opaque' && _.has(obj.data, 'WebHookSecretKey') && Object.keys(obj.data).length === 1) {


I would use

if (_.has(obj, 'data.WebHookSecretKey') && _.size(obj.data) === 1) {

The whole function could be simplified to

// Edit in YAML if not editing a webhook secret with one key. const editInYaml = obj => !_.has(obj, 'data.WebHookSecretKey') || _size(obj.data) !== 1;

We should make the string WebHookSecretKey a const defined once somewhere that we reuse to avoid typos.

spadgett · 2018-06-19T16:21:54Z

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

+      secret: secret,
+      inProgress: false,
+      type: secret.type,
+      stringData: _.mapValues(secret.data, (v) => window.atob(v)),


I think this can be

stringData: _.mapValues(secret.data, window.atob),

spadgett · 2018-06-19T16:22:27Z

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

+      type: secret.type,
+      stringData: _.mapValues(secret.data, (v) => window.atob(v)),
+    };
+    this.onNameChanged = this.onNameChanged.bind(this);


You should bind onDataChanged here, too

spadgett · 2018-06-19T16:23:24Z

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

+    e.preventDefault();
+    const { kind, metadata } = this.state.secret;
+    this.setState({ inProgress: true });
+    const newSecret = _.assign({}, this.state.secret, {stringData: this.state.stringData});


Does this include both data and stringData? We should only include stringData

spadgett · 2018-06-19T16:26:31Z

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

+    const title = `${this.props.titleVerb} ${_.upperFirst(this.state.secretType)} Secret`;
+    const { saveButtonText } = this.props;
+
+    let explanation = 'Webhook secret allow you to authenticate a webhook trigger.';


'Webhook secrets allow you to authenticate a webhook trigger.'

spadgett · 2018-06-19T16:38:15Z

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

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


It would be nice if the base edit component didn't need to know about the specific secret types. I wonder if we can't use a higher-order component here:

https://reactjs.org/docs/higher-order-components.html#use-hocs-for-cross-cutting-concerns

spadgett · 2018-06-19T16:41:24Z

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

+
+const BaseEditSecret = connect(null, {setActiveNamespace: UIActions.setActiveNamespace})(
+  (props: BaseEditSecretProps_) => <BaseEditSecret_ {...props} />
+);


Can't this just be...

const BaseEditSecret = connect(null, {setActiveNamespace: UIActions.setActiveNamespace})(BaseEditSecret_);

No the props need to be passed explicitly from parent to child otherwise, yarn will scream that some properties does not exist:

Property 'fixed' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<Pick<any, never>, ComponentState, never>...'.

Then I think we must have declared the types incorrectly somehow. We should figure out what's wrong.

The issue is that the redux part of the code is in jsx an therefor is not typed, eg. ui-actions.js. I think until we type the redux part of the then codebase, which means rewriting it to TS, until then we need this as a workaround.

Was trying this workaround but due to issue described above it havent worked.

spadgett · 2018-06-19T16:42:05Z

frontend/public/components/secrets/subforms.tsx

+const generateSecret = () => {
+  // http://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript
+  const s4 = () => Math.floor((1 + Math.random()) * 0x10000).toString(16).substring(1);
+  return s4()+s4()+s4()+s4();


nit: spaces before and after +

spadgett · 2018-06-19T16:43:26Z

frontend/public/components/utils/button-bar.jsx

+    {props.errorMessage && <ErrorMessage message={props.errorMessage} />}
+    {injectDisabled(props.children, props.inProgress)}
+    {props.inProgress && <LoadingInline />}
+    {props.infoMessage && <InfoMessage message={props.infoMessage} />}


It's not clear to me why this change was needed?

There was an issue with the required properties for the ButtonBar

TS2322: Type '{ children: Element[]; errorMessage: any; inProgress: boolean; }' is not assignable to type 'IntrinsicAttributes & { children: any; className: any; errorMessage: any; infoMessage: any; inPro...'.
Type '{ children: Element[]; errorMessage: any; inProgress: boolean; }' is not assignable to type '{ children: any; className: any; errorMessage: any; infoMessage: any; inProgress: any; }'.
Property 'className' is missing in type '{ children: Element[]; errorMessage: any; inProgress: boolean; }'.
Version: typescript 2.7.2
Time: 12738ms

It was cause by the listed params in the class definition

export const ButtonBar = ({children, className, errorMessage, infoMessage, inProgress}) => {
...
}

Because of listing the parameters and not just setting the to props, all the listed params will be required, even if propTypes will declare that they are not.

Can you try adding an @type comment instead and mark className option?

https://github.com/openshift/console/blob/master/frontend/public/components/factory/list-page.jsx#L24

benjaminapetersen · 2018-06-19T17:28:24Z

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

+
+const determineSecretTypeAbstraction = (secret) => {
+  const { data } = secret;
+  if (_.has(data, 'WebHookSecretKey')) {


Style nit, but I think you will see something like:

return _.has(data, 'WebHookSecretKey') ? 'webhook' : 'generic';

Throughout the app. If possible I think we should try to conform.

benjaminapetersen

Just a few more minor tweaks!

benjaminapetersen · 2018-06-19T17:29:35Z

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

+class BaseEditSecret_ extends SafetyFirst<BaseEditSecretProps_, BaseEditSecretState_> {
+  constructor (props) {
+    super(props);
+    let existingData = _.pick(props.obj, ['metadata', 'data']);


I think you can const existingData. It isn't changing.

benjaminapetersen · 2018-06-19T17:29:59Z

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

+    this.save = this.save.bind(this);
+  }
+  onDataChanged (secretsData) {
+    let stringData = {...this.state.stringData};


Also, you are defining stringData then immediately setting it as something else.

Could this just be:

this.setState({... secretsData});

benjaminapetersen · 2018-06-19T17:31:56Z

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

+    this.setState({stringData});
+  }
+  onNameChanged (event) {
+    let secret = {...this.state.secret};


es6:

this.setState({ ...this.state.secret, metadata: { name: event.target.value } });

That will just set the name in the metadata field, everything else will be removed when set. Tried:

this.setState({ ...this.state.secret, metadata: { name: event.target.value ...this.state.secret.namespace, } });

but thats not setting the name at all

benjaminapetersen · 2018-06-19T17:35:49Z

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

+    (this.props.isCreate
+      ? k8sCreate(ko, newSecret)
+      : k8sUpdate(ko, newSecret, metadata.namespace, newSecret.metadata.name)
+    ).then(


Mind collapsing some parens? Ex: .then(() => {.

benjaminapetersen · 2018-06-19T17:38:39Z

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

+</Firehose>;
+
+/* eslint-disable no-undef */
+export type BaseEditSecretState_ = {


Is the trailing underscore common? I haven't seen previously.

It is, at least we use it in our codebase

Huh, ok neat.

Not typically for the types, though?

Probably not. If anything we could rename:

BaseEditSecret -> BaseEditSecretWrapper

BaseEditSecret_ -> BaseEditSecret

I think the names are good. Just wondering if we should have the underscore on the type definitions

benjaminapetersen · 2018-06-19T17:48:06Z

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

+import { SafetyFirst } from '../safety-first';
+import { WebHookSecretSubform } from './subforms';
+
+const determineSecretTypeAbstraction = (secret) => {


Might be fine to name this determineSecretType() or just secretType().

Secrets are a kind already have type field, and all the values that could return from that function will be just an type abstractions:

webhook

source

image

key/value

Ok makes sense.

benjaminapetersen · 2018-06-19T17:54:21Z

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

+  kind?: string,
+  isCreate: boolean,
+  titleVerb: string,
+  setActiveNamespace: any,


setActiveNamespace must be a function, correct? I believe this can be typed.

benjaminapetersen · 2018-06-19T18:00:03Z

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

+  error?: any,
+};
+
+export type BaseEditSecretProps_ = {


Can we eliminate the any types?
For example:

obj: K8sResourceKind // ... metadata: { name: string, // etc. }

I think this file has some better type arbitrary examples.

fixed can really be any... It's just saying always use these properties. I don't think we can define that one.

We can use K8sResourceKind for obj

Can we eliminate the any types?

no all the types have to be typed, otherwise yarn will screen, eg:

Property 'metadata' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<Pick<BaseEditSecretProps_, "fixed" | "ki...'

Since some parts of codebase are not typed the its almost impossible not to use any

benjaminapetersen · 2018-06-19T18:00:44Z

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

+    const title = `${this.props.titleVerb} ${_.upperFirst(this.state.secretType)} Secret`;
+    const { saveButtonText } = this.props;
+
+    let explanation = 'Webhook secret allow you to authenticate a webhook trigger.';


const explanation;
const subform;

stevekuznetsov · 2018-06-19T18:41:35Z

/test backend
/test frontend

jhadvig · 2018-06-19T20:37:17Z

@spadgett @benjaminapetersen comments addressed.
Regarding the Higher order composition comment I'm still looking into a way how that could be done, but not sure if it will be a benefit for us.

spadgett · 2018-06-19T20:39:24Z

Regarding the Higher order composition comment I'm still looking into a way how that could be done, but not sure if it will be a benefit for us.

OK to leave for now, but it's something that we might explore before adding all of the dialogs when it's harder to change.

spadgett · 2018-06-19T20:57:59Z

frontend/integration-tests/tests/crud.scenario.ts

-        await crudView.createYAMLButton.click();
+        const exists = await crudView.createItemButton.isPresent();
+        if (exists) {
+          console.log('true');


Remove console.log here and just below

spadgett · 2018-06-19T21:00:17Z

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

+import { connect } from 'react-redux';
+import { Link } from 'react-router-dom';
+
+import { WEBHOOKSECRETKEY } from '../../const';


I would probably just export it in this file.

export const WebHookSecretKey = 'WebHookSecretKey';

spadgett · 2018-06-19T21:01:15Z

frontend/public/components/secrets/subforms.tsx

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


These components are going to be so tightly coupled to create secret, I would keep them in the same file. Then you don't need to export them since they're internal to the implementation.

spadgett · 2018-06-19T21:02:15Z

frontend/public/components/secrets/subforms.tsx

+export class WebHookSecretSubform extends React.Component<WebHookSecretSubformProps, WebHookSecretSubformState> {
+  constructor(props) {
+    super(props);
+    this.state = {WebHookSecretKey: this.props.stringData.WebHookSecretKey || ''};


nit: I'd call this webhookSecretKey. State properties usually begin with lowercase letters

true but this key is kinda specific, since it the only one from the known secret keys that starts with uppercase. Also the this.state of WebHookSecretSubform is mapped to this.state.secret.stringData of BaseEditSecret_, therefor we would need to handle the name conversion somewhere else eventually.

spadgett · 2018-06-19T21:02:37Z

frontend/public/components/utils/button-bar.jsx

@@ -21,6 +21,7 @@ const InfoMessage = ({message}) => <div className="alert alert-info"><span class
 // NOTE: DO NOT use <a> elements within a ButtonBar.
 // They don't support the disabled attribute, and therefore
 // can't be disabled during a pending promise/request.
+/** @type {React.SFC<{children: any, className?: string, errorMessage?: string, infoMessage?: string, inProgress: boolean}}>} */


spadgett · 2018-06-19T21:06:38Z

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

+
+const BaseEditSecret = connect(null, {setActiveNamespace: UIActions.setActiveNamespace})(
+  (props: BaseEditSecretProps_) => <BaseEditSecret_ {...props} />
+);


Then I think we must have declared the types incorrectly somehow. We should figure out what's wrong.

spadgett · 2018-06-19T21:08:34Z

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

+</Firehose>;
+
+/* eslint-disable no-undef */
+export type BaseEditSecretState_ = {


Not typically for the types, though?

spadgett · 2018-06-19T21:09:07Z

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

+
+/* eslint-disable no-undef */
+export type BaseEditSecretState_ = {
+  secretType?: string,


This would be better as an enum

beanh66 · 2018-06-20T12:02:03Z

@jhadvig Do you mind adding a screenshot of the latest? Thanks!

jhadvig · 2018-06-20T12:35:00Z

@beanh66 there haven't been much of a changes, just wording in the form:

beanh66 · 2018-06-20T12:48:21Z

@jhadvig Thanks! Question...should the last screen say "Edit Secret" as the title? Wondering why we need Generic.

jhadvig · 2018-06-20T13:10:35Z

@beanh66 sorry for that, accidentally d'n'd old screen there.. the title should reflect the secret type, in this case it should be - Edit Webhook Secret, the same for create page - Create Webhook Secret

jhadvig · 2018-06-20T15:26:52Z

@spadgett comments addressed and comments added. PTAL

spadgett

👍

Thanks for your patience, @jhadvig. LGTM

spadgett · 2018-06-20T18:19:28Z

/lgtm
/retest

I suspect the ci/prow/* jobs are broken again, but let's check

openshift-ci-robot · 2018-06-20T18:20:59Z

@jhadvig: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/crud	`a6383dd`	link	`/test crud`
ci/prow/alm	`a6383dd`	link	`/test alm`
ci/prow/performance	`a6383dd`	link	`/test performance`
ci/prow/backend	`33d1cba`	link	`/test backend`
ci/prow/frontend	`33d1cba`	link	`/test frontend`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jhadvig · 2018-06-20T21:42:18Z

Thanks @spadgett much appreciated ! :)

…penshift#94)

…w_shared Migration: Add shared components

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 5, 2018

spadgett reviewed Jun 5, 2018

View reviewed changes

jhadvig force-pushed the secret-form branch from 16aec11 to 6f4183a Compare June 13, 2018 17:33

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2018

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 13, 2018

jhadvig force-pushed the secret-form branch from 6f4183a to 3e11a5a Compare June 13, 2018 17:45

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2018

jhadvig force-pushed the secret-form branch 3 times, most recently from e0f29ab to 599edb8 Compare June 13, 2018 18:07

jhadvig changed the title ~~[WIP] Create secret page~~ Create secret page Jun 13, 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 13, 2018

alecmerdler suggested changes Jun 14, 2018

View reviewed changes

jhadvig changed the title ~~Create secret page~~ Create/Edit page for Webhook secrets Jun 14, 2018

jhadvig force-pushed the secret-form branch from 599edb8 to 62cfb7f Compare June 14, 2018 11:07

spadgett requested changes Jun 15, 2018

View reviewed changes

jhadvig force-pushed the secret-form branch from edc3b64 to a6383dd Compare June 19, 2018 07:45

spadgett requested changes Jun 19, 2018

View reviewed changes

benjaminapetersen reviewed Jun 19, 2018

View reviewed changes

benjaminapetersen suggested changes Jun 19, 2018

View reviewed changes

jhadvig force-pushed the secret-form branch from a6383dd to 1a23d71 Compare June 19, 2018 20:32

spadgett requested changes Jun 19, 2018

View reviewed changes

Create secret page

33d1cba

jhadvig force-pushed the secret-form branch from 1a23d71 to 33d1cba Compare June 20, 2018 15:22

spadgett approved these changes Jun 20, 2018

View reviewed changes

openshift-ci-robot assigned spadgett Jun 20, 2018

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

spadgett merged commit add3636 into openshift:master Jun 20, 2018

jhadvig mentioned this pull request Jun 22, 2018

Create/Edit source secret #150

Merged

christianvogt pushed a commit to christianvogt/console that referenced this pull request May 16, 2019

Fix form validation and redirect to topology page in dev perspective (o…

72f34c5

…penshift#94)

TimothyAsirJeyasing pushed a commit to TimothyAsirJeyasing/console that referenced this pull request Aug 3, 2022

Merge pull request openshift#94 from SanjalKatiyar/migrate_wizard_flo…

a91f773

…w_shared Migration: Add shared components

Create/Edit page for Webhook secrets #94

Create/Edit page for Webhook secrets #94

Conversation

jhadvig commented Jun 5, 2018 • edited Loading

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

kans Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

spadgett commented Jun 5, 2018

robszumski commented Jun 6, 2018

kans commented Jun 6, 2018

spadgett commented Jun 7, 2018

spadgett commented Jun 7, 2018

cshinn commented Jun 8, 2018

jhadvig commented Jun 13, 2018

cshinn commented Jun 13, 2018

beanh66 commented Jun 13, 2018 • edited Loading

alecmerdler 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

jhadvig Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Jun 14, 2018 • edited Loading

beanh66 commented Jun 15, 2018

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

spadgett commented Jun 15, 2018

jhadvig commented Jun 18, 2018

spadgett commented Jun 18, 2018

spadgett commented Jun 18, 2018

jhadvig commented Jun 19, 2018 • edited Loading

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

jhadvig Jun 20, 2018 • edited Loading

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

benjaminapetersen 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

jhadvig Jun 19, 2018 • edited Loading

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

jhadvig commented Jun 5, 2018 •

edited

Loading

kans Jun 6, 2018 •

edited

Loading

beanh66 commented Jun 13, 2018 •

edited

Loading

jhadvig Jun 14, 2018 •

edited

Loading

jhadvig commented Jun 14, 2018 •

edited

Loading

jhadvig commented Jun 19, 2018 •

edited

Loading

jhadvig Jun 20, 2018 •

edited

Loading

jhadvig Jun 19, 2018 •

edited

Loading