Allow EnvFrom Prefix by cdcabrera · Pull Request #2377 · openshift/origin-web-console

cdcabrera · 2017-10-25T17:42:22Z

Opened envFrom to allow prefix input.

Minor display logic updates
Design/UX modifications

Related to #2182 and closes #2362

cdcabrera · 2017-10-26T14:06:08Z

app/styles/_kve.less

 .key-value-editor .key-value-editor-input,
-.key-value-editor-header {
+.key-value-editor-header,
+.environment-from-editor-header {


@sg00dwin made adjustment for the column widths see PR screenshot. Still looking like the column containing the prefix field is a few pixels off compared to the ui-select column above

spadgett

It'd probably be good for EnvironmentService compact method to remove empty prefixes.

spadgett · 2017-10-26T17:04:26Z

app/scripts/directives/editEnvironmentFrom.js

      }
    };

+    ctrl.removePrefix = function (entry) {


Should no longer be needed.

Agreed, on the clean up list

spadgett · 2017-10-26T17:06:04Z

app/views/directives/edit-environment-from.html

+        <small class="pficon pficon-help"
+               aria-hidden="true"
+               data-toggle="tooltip"
+               data-original-title="Optional prefix added to each environment variable name for this resource."></small>


Maybe remove "for this resource." since it's a column header with many resources.

I know I'm picking nits on my own suggested wording :)

agree, drop the "for this resource"

spadgett · 2017-10-26T17:10:03Z

app/views/directives/edit-environment-from.html

+              {{entry.configMapRef.name}}</span>
+            <span ng-if="entry.secretRef.name">secret
+              <span ng-if="entry.prefix">"{{entry.prefix}}"</span>
+              {{entry.secretRef.name}}</span>


Maybe...?

Use all keys and values from config map my-config-map, adding name prefix "my-prefix-"

@jwforres is better at wording stuff then me, though.

tough to stuff that into one sentence, would probably read better as two.

"Use all keys and values from config map my-config-map. Names will be prefixed with "my-prefix".

spadgett · 2017-10-26T17:11:04Z

app/views/directives/edit-environment-from.html

+                 placeholder="Add prefix"
+                 id="envfrom-prefix-{{$index}}"
+                 name="envfrom-prefix-{{$index}}"
+                 ng-model="entry.prefix"/>


We usually don't include the self-closing /

Old habits, can remove

spadgett · 2017-10-26T17:11:41Z

app/views/directives/edit-environment-from.html

+      <div class="form-group environment-from-input">
+        <div class="environment-from-input" ng-if="!$ctrl.isEnvFromReadonly(entry) && $ctrl.hasOptions()">
+          <label for="envfrom-prefix-{{$index}}" class="sr-only">Prefix</label>
+          <input type="text"


This should have a pattern with a corresponding validation error. (I'll need to look up the correct regex for prefix.)

That works, yeah send it over

spadgett · 2017-10-26T17:17:19Z

@sg00dwin Can you help with the suggested style changes in #2362? (OK as a separate PR.)

spadgett · 2017-10-26T17:22:51Z

Looks like prefix has to match the env name regex

https://github.com/kubernetes/kubernetes/blob/5f030f4568822e791054d5c4c3aff7f9f5c85151/pkg/api/validation/validation.go#L1808-L1812

We're using [A-Za-z_][A-Za-z0-9_]*

cdcabrera · 2017-10-27T03:59:22Z

@beanh66 @jwforres @spadgett

Prefix and the modal display, is this something we should try to work in?

spadgett · 2017-10-27T13:39:31Z

@cdcabrera Are you asking if we should show the prefix in the modal? I would just show the names as they are defined in the secret.

cdcabrera · 2017-10-27T14:06:20Z

@spadgett indeed was wondering if the prefix needed to be displayed in some fashion within the modal itself

Opened envFrom to allow prefix input.

cdcabrera · 2017-10-27T16:03:36Z

@spadgett per request removed the extra component bindings. Should now only have the main ones

entries
envFromSelectorOptions
isReadonly

Also as noted the "ng-pattern" copied over from key/values component misses out on special characters tacked onto the end, corrected it. There's still some funkiness around empty strings that happens on both Key/Values and EnvFrom

@sg00dwin your styling updates have been merged into this PR

spadgett · 2017-10-30T13:27:12Z

I feel like in read-only mode we should use the same two column display instead of trying to write things out as sentences. Also I don't think we need to use break-all, which breaks in the middle of words when not needed. We might look at this as a follow on, however, to get the blocker fixed.

spadgett

Code looks good, and this fixes the fundamental bug. We can look at any additional changes to presentation as follow-on work.

spadgett · 2017-10-30T13:35:29Z

/lgtm

openshift-merge-robot · 2017-10-30T13:58:24Z

Automatic merge from submit-queue.

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2017

cdcabrera force-pushed the issue-envfrom-prefix branch from a3c0ced to a6b6b19 Compare October 25, 2017 21:47

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2017

cdcabrera force-pushed the issue-envfrom-prefix branch from a6b6b19 to 26c953a Compare October 25, 2017 22:05

cdcabrera requested review from benjaminapetersen, sg00dwin and spadgett October 26, 2017 14:03

cdcabrera commented Oct 26, 2017

View reviewed changes

spadgett requested changes Oct 26, 2017

View reviewed changes

spadgett added the kind/bug Categorizes issue or PR as related to a bug. label Oct 26, 2017

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2017

cdcabrera force-pushed the issue-envfrom-prefix branch from 944e8ab to 26622e7 Compare October 27, 2017 03:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2017

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 Oct 27, 2017

cdcabrera force-pushed the issue-envfrom-prefix branch from 26622e7 to 343cf64 Compare October 27, 2017 15:53

Allow EnvFrom Prefix

bafca94

Opened envFrom to allow prefix input.

cdcabrera force-pushed the issue-envfrom-prefix branch from 343cf64 to bafca94 Compare October 27, 2017 15:57

cdcabrera changed the title ~~[WIP] Allow EnvFrom Prefix~~ Allow EnvFrom Prefix Oct 27, 2017

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 Oct 27, 2017

spadgett approved these changes Oct 30, 2017

View reviewed changes

openshift-ci-robot assigned spadgett Oct 30, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2017

openshift-merge-robot merged commit 39494d0 into openshift:master Oct 30, 2017

Conversation

cdcabrera commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdcabrera Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

spadgett commented Oct 26, 2017

Uh oh!

spadgett commented Oct 26, 2017

Uh oh!

cdcabrera commented Oct 27, 2017

Uh oh!

spadgett commented Oct 27, 2017

Uh oh!

cdcabrera commented Oct 27, 2017

Uh oh!

cdcabrera commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Oct 30, 2017

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett commented Oct 30, 2017

Uh oh!

openshift-merge-robot commented Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cdcabrera commented Oct 25, 2017 •

edited

Loading

cdcabrera Oct 26, 2017 •

edited

Loading

cdcabrera commented Oct 27, 2017 •

edited

Loading