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

Need to handle envFrom prefix #2362

Closed
spadgett opened this issue Oct 24, 2017 · 10 comments · Fixed by #2377
Closed

Need to handle envFrom prefix #2362

spadgett opened this issue Oct 24, 2017 · 10 comments · Fixed by #2377
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/P1
Milestone

Comments

@spadgett
Copy link
Member

Our environment from implementation does not handle the prefix. We need to make sure we're not dropping values with different prefixes minimally, but we should also let users see and change the prefix values.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L1698-L1700

@spadgett spadgett added kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Oct 24, 2017
@spadgett spadgett added this to the 3.7.0 milestone Oct 24, 2017
@spadgett spadgett self-assigned this Oct 24, 2017
@spadgett
Copy link
Member Author

@jwforres @cdcabrera FYI

@spadgett
Copy link
Member Author

@beanh66

@cdcabrera
Copy link
Contributor

cdcabrera commented Oct 25, 2017

@spadgett @beanh66 @jwforres Initial work, design input requested...

Implemented a quick fix layout wise to get the code working, realize it's not pretty but should allow for opening the discussion...

screen shot 2017-10-25 at 1 27 22 pm

and a quick animation...

oct-25-2017 13-25-42

@spadgett
Copy link
Member Author

I was thinking of having the Prefix input always visible. If it's blank, no prefix is set. Something like

Config Map / Secret     Prefix

[Select resource... ]   ________________   <drag> <x> View Details
[Select resource... ]   ________________   <drag> <x> View Details
...

@cdcabrera cdcabrera mentioned this issue Oct 25, 2017
2 tasks
@beanh66
Copy link
Member

beanh66 commented Oct 25, 2017

@spadgett I like that suggestion. Wondering if we can at least add the field level help icon to the right of the "Prefix" label so users can hover to find out what a prefix is for. Do you have a blurb to define that input already?

@spadgett
Copy link
Member Author

Wondering if we can at least add the field level help icon to the right of the "Prefix" label so users can hover to find out what a prefix is for.

Yeah, I think a tooltip makes sense.

Do you have a blurb to define that input already?

Maybe

"Optional prefix added to each environment variable name for this resource."

?

@cdcabrera
Copy link
Contributor

Updated towards the recommendation @spadgett @beanh66

screen shot 2017-10-25 at 5 06 37 pm

@spadgett spadgett assigned cdcabrera and unassigned spadgett Oct 26, 2017
@beanh66
Copy link
Member

beanh66 commented Oct 26, 2017

@cdcabrera The new prefix stuff looks good to me!

As an aside, the Environment From text looks oddly small to me. @jennyhaines I know we were trying to use that style consistently in font size 10, but it looks off in this context. Thoughts? (Note, this is a separate "Environment From" section, but the "Container node-ex" label still applies to both the top and bottom sections)

@jennyhaines
Copy link

@beanh66 @cdcabrera @spadgett - I see what you're saying about the oddly small "Environment From" text, Colleen. To ensure the right hierarchy between the larger "Container node-ex" grouping, and the "Environment From" subgrouping, I have some suggested changes, labeled in the attached photo. Thoughts? (Also, I wonder if there is better wording for "Environment From" group?)
recommendation

@spadgett
Copy link
Member Author

cc @sg00dwin

openshift-merge-robot added a commit that referenced this issue Oct 30, 2017
Automatic merge from submit-queue.

Allow EnvFrom Prefix

Opened envFrom to allow prefix input.

- [x] Minor display logic updates
- [x] Design/UX modifications

Related to #2182 and closes #2362 

![screen shot 2017-10-25 at 5 06 37 pm](https://user-images.githubusercontent.com/3761375/32026101-d15b6e5c-b9b0-11e7-8405-509d2931056e.png)
sg00dwin added a commit to sg00dwin/origin-web-console that referenced this issue Oct 30, 2017
sg00dwin added a commit to sg00dwin/origin-web-console that referenced this issue Oct 30, 2017
sg00dwin added a commit to sg00dwin/origin-web-console that referenced this issue Oct 31, 2017
sg00dwin added a commit to sg00dwin/origin-web-console that referenced this issue Nov 1, 2017
sg00dwin added a commit to sg00dwin/origin-web-console that referenced this issue Nov 1, 2017
openshift-merge-robot added a commit that referenced this issue Nov 1, 2017
Automatic merge from submit-queue.

Follow-on updates from jennyhaines

Requested changes
#2362 (comment) and #2182 (comment)


<img width="755" alt="screen shot 2017-10-30 at 3 49 34 pm" src="https://user-images.githubusercontent.com/1874151/32193045-65a72b30-bd8c-11e7-9704-c763ab161392.png">

<img width="326" alt="screen shot 2017-10-30 at 4 08 01 pm" src="https://user-images.githubusercontent.com/1874151/32193095-8c67a470-bd8c-11e7-832b-6dc4914311ce.png">
f0x11 pushed a commit to f0x11/origin-web-console that referenced this issue Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/P1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants