[CDAP-13236] Allows editing of keytabURI of a namespace in UI #10006

tonybach · 2018-04-02T21:46:46Z

JIRA: https://issues.cask.co/browse/CDAP-13236

Backend PR: #9701

Allows editing of keytabURI property of a namespace in UI, from its Namespace Details page. Currently only keytabURI is editable since we don't have backend code to allow editing of principal yet.

elfenheart · 2018-04-03T00:20:27Z

cdap-ui/app/cdap/components/NamespaceDetails/Security/index.js

+        <div className="namespace-details-section-label">
+          <strong>{T.translate(`${PREFIX}.label`)}</strong>
+          {
+            this.props.principal && this.props.keytabURI ?


Why do we need this check? I can't edit if the namespace initially created without these values?

Yes currently we can't edit the keytabURI if the namespace was created without keytabURI and principal properties.

elfenheart · 2018-04-03T00:21:45Z

cdap-ui/app/cdap/components/CaskWizards/AddNamespace/PreferencesStep/index.js

@@ -24,7 +24,8 @@ export default function PreferencesStep() {

  const mapStateToKeyValProps = (state) => {
    return {
-      keyValues : state.preferences.keyValues
+      keyValues : state.preferences.keyValues,
+      disabled: state.editableFields.fields.indexOf('keyValues') === -1


shouldn't the field name be preferences?

ajainarayanan

A few minor comments.

ajainarayanan · 2018-04-03T00:17:50Z

cdap-ui/app/cdap/services/WizardStores/AddNamespace/AddNamespaceStore.js

@@ -17,6 +17,7 @@
 import {combineReducers, createStore, compose} from 'redux';
 import AddNamespaceActions from 'services/WizardStores/AddNamespace/AddNamespaceActions';
 import AddNamespaceWizardConfig from 'services/WizardConfigs/AddNamespaceWizardConfig';
+import {convertMapToKeyValuePairsObj} from 'components/KeyValuePairs/KeyValueStoreActions';


nit: May be we should extract tis to services/helpers. We seem to be using this in a lot of places outside of KeyValuePairs

This conversion still has a lot of specific behavior for KeyValuePairs component, such as the unique id and also if it's empty, we want to set a default. So I don't think it should sit in helpers.

May be extract the functionality of converting a map to key, value? May be the function can accept another argument to add unique ids to each key value (since when we render key-value anywhere in the DOM we need a unique id)?
we want to set a default this is the part that becomes a little out of place for me. We seem to be using a default state of a component inside the store of another component. May be I am missing something.

Have extracted out the functionality of converting a map to a key-value pairs array.

ajainarayanan · 2018-04-03T00:21:13Z

cdap-ui/app/cdap/services/WizardStores/AddNamespace/AddNamespaceStore.js

@@ -60,6 +61,21 @@ const defaultPreferencesState = Object.assign({
  }
 }, skippableDefaultState);

+const defaultEditableFieldsState = Object.assign({
+  fields: [
+    'name',


Can this be,

Object.keys({ ...defaultGeneralState, ...defaultSecurityState, ...defaultPreferencesState })

I am afraid tomorrow we might add a field to one of the other objects and forget to add here. Removes the necessity to hard code the same property in two places.

Also for simplicity can the editable fields be an array? do we need editableFields.fields?

We still want it to be an object, since we want to pass the __complete: true property to it. This lets the Wizard component know that the step is complete, so the 'Finish' button can be enabled in the wizard. This is done in the checkRequiredSteps() method in the Wizard component.

ajainarayanan · 2018-04-03T00:25:34Z

cdap-ui/app/cdap/components/CaskWizards/AddNamespace/index.js

+
+  static defaultProps = {
+    isEdit: false,
+    editableFields: [],


should this default from the store?

ajainarayanan · 2018-04-03T00:31:05Z

cdap-ui/app/cdap/components/CaskWizards/AddNamespace/index.js

+    if (this.props.isEdit) {
+      return editNamespaceProperties();
+    } else {
+      return this.createNamespace();


should this use PublishNamespace from the namespace action creator?

Internally this.createNamespace() does use PublishNamespace(). Have renamed this.createNamespace() to this.publishNamespaceAndPreferences() to be more clear.

ajainarayanan · 2018-04-03T00:58:04Z

cdap-ui/app/cdap/services/WizardStores/AddNamespace/ActionCreator.js

+  return publishOrEditNamespace(MyNamespaceApi.create);
+};
+
+const editNamespaceProperties = () => {


nit: Should this be EditNamespaceProperties? to be consistent with PublishNamespace? Or change publishNamespace or createNamespace

ajainarayanan

Minor comments. Otherwise 👍 LGTM

ajainarayanan · 2018-04-04T00:23:46Z

cdap-ui/app/cdap/services/helpers.js

+    return {
+      key: mapKey,
+      value: map[mapKey],
+      uniqueId: 'id-' + uuid()


should this not be here? Also we can start to use Object.entries(map).map(([key, value]) => {...}) makes it simpler.

ajainarayanan · 2018-04-04T00:31:10Z

cdap-ui/app/cdap/services/WizardStores/AddNamespace/ActionCreator.js

@@ -16,7 +16,15 @@
 import AddNamespaceStore from 'services/WizardStores/AddNamespace/AddNamespaceStore';
 import {MyNamespaceApi} from 'api/namespace';

-const PublishNamespace = () => {
+const publishNamespace = () => {


nit: May be just saveNamespace? and savePreferences? Don't know if we could say 'publish namespace' here.

tonybach · 2018-04-06T22:41:55Z

Addressed comments, rebasing and merging.

- Adds Edit label, populates wizard when clicked on, and navigates to Security tab in wizard - Adds api to edit namespace when user clicks Finish in the wizard - Only allows editing of keytabURI if the specified some values for principal and keytabURI during namespace creation - Resets namespace wizard after finishing edit

tonybach added UI 5.0 labels Apr 2, 2018

elfenheart reviewed Apr 3, 2018

View reviewed changes

ajainarayanan reviewed Apr 3, 2018

View reviewed changes

tonybach added the On-hold label Apr 3, 2018

ajainarayanan reviewed Apr 4, 2018

View reviewed changes

tonybach force-pushed the feature/ui-cdap-13236-update-keytab-uri branch from 909bf19 to 1dd324d Compare April 6, 2018 22:58

tonybach removed the On-hold label Apr 6, 2018

tonybach merged commit c24c83f into develop Apr 6, 2018

tonybach deleted the feature/ui-cdap-13236-update-keytab-uri branch April 6, 2018 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDAP-13236] Allows editing of keytabURI of a namespace in UI #10006

[CDAP-13236] Allows editing of keytabURI of a namespace in UI #10006

tonybach commented Apr 2, 2018 •

edited

Loading

elfenheart Apr 3, 2018

tonybach Apr 3, 2018

elfenheart Apr 3, 2018

ajainarayanan left a comment

ajainarayanan Apr 3, 2018

elfenheart Apr 3, 2018

ajainarayanan Apr 3, 2018

tonybach Apr 3, 2018

ajainarayanan Apr 3, 2018

ajainarayanan Apr 3, 2018

tonybach Apr 3, 2018

ajainarayanan Apr 3, 2018

ajainarayanan Apr 3, 2018

tonybach Apr 3, 2018

ajainarayanan Apr 3, 2018

ajainarayanan left a comment

ajainarayanan Apr 4, 2018

ajainarayanan Apr 4, 2018

tonybach commented Apr 6, 2018

[CDAP-13236] Allows editing of keytabURI of a namespace in UI #10006

[CDAP-13236] Allows editing of keytabURI of a namespace in UI #10006

Conversation

tonybach commented Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajainarayanan 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

Choose a reason for hiding this comment

ajainarayanan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonybach commented Apr 6, 2018

tonybach commented Apr 2, 2018 •

edited

Loading