-
Notifications
You must be signed in to change notification settings - Fork 230
Let users define labels when creating routes #985
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
Conversation
|
I can't decide on the system labels here, I see the argument for it, but at the same time we don't let you remove the system labels. |
|
I don't disagree, but I think it's a problem in general with how we approach system labels. What if I don't want an |
|
i think the difference that bothers me here is that in other cases they are really system labels, in this case its not just copying the system labels like the app label, its copying all the labels from the service, some of which may have been user defined |
|
I could prefill them as user-defined labels, but it gets tricky if the user changes services in the dropdown. Or maybe a "Copy from Service" link? |
|
copy from service link seems reasonable, then its easy without being forced into it |
662f391 to
14635d5
Compare
|
Updated with a "Copy Service Labels" link. |
14635d5 to
7513f20
Compare
jwforres
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit, otherwise LGTM
app/views/create-route.html
Outdated
| can-toggle="false" | ||
| help-text="Labels for this route."> | ||
| </label-editor> | ||
| <a href="" ng-click="copyServiceLabels()">Copy service labels</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headline case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right above this link is "Add label" (at least until #978 merges)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k i'll leave that up to you which order you want to fix and merge the two PRs in :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge the other one first, and I'll rebase this one. About to push updates for #978
7513f20 to
e8af265
Compare
|
Updated to headline case. [merge] |
|
Evaluated for origin web console merge up to e8af265 |
|
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/803/) (Base Commit: 6cbd1a7) |
Fixes openshift/origin#6095
@jwforres PTAL