-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Deployment configs on the web console, with deployments grouped #2178
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
Deployment configs on the web console, with deployments grouped #2178
Conversation
assets/app/views/deployments.html
Outdated
| </div> | ||
| </dl> | ||
|
|
||
| <dl class="dl-horizontal left indent"> |
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.
move the manual ones first, since they're the ones you can actually act on.
f30c4e7 to
ff4ebea
Compare
|
I just pushed a few improvements and a new screenshot that reflects them. |
04c4422 to
42b11b1
Compare
|
cc @spadgett |
|
Fixes #2145 |
|
I realize you're mirroring the Builds page, but it seems strange that the Start Build button is presented like a property value. I'd rather see it by itself below Triggers or maybe in the upper right. Agree with @liggitt: I'm not sure we need the command-line example if we have the button. We'd want to make the same change on the Builds page if we change it here. Not specific to your change, but the pod template for each deployment feels really crowded with no top margin above it. Overall, however, this looks much better. |
|
Do your changes work with label filters? |
42b11b1 to
bd90f66
Compare
|
Most comments addressed. No "Deploy" button for now, that will be part of a separate story (we need to be able to handle PUT|PATCH on the web console first). Working fine with label filters. @liggitt @spadgett @ironcladlou please review. |
610d765 to
5a42299
Compare
|
UP |
3281408 to
1afbfd9
Compare
97eb7d8 to
b32524e
Compare
|
Fixed recent annotation changes. |
955b478 to
0ceec90
Compare
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/1943/) (Image: devenv-fedora_1518) |
| <dd ng-repeat="(selectorLabel, selectorValue) in deploymentConfig.template.controllerTemplate.replicaSelector">{{selectorLabel}}={{selectorValue}}<span ng-show="!$last">, </span></dd> | ||
| <dt>Replicas:</dt> | ||
| <dd> | ||
| {{deploymentConfig.template.controllerTemplate.replicas}} |
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.
are you switching back to v1beta1 syntax here?
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.
ah, osapi v1beta1... hrmm... don't really want to add something we're going to have to immediately rework when we switch the UI to osapi v1beta3 (which is happening asap, right @smarterclayton?)
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.
I've talked myself into thinking this page is just one of many places we'll have to update, so the api version thing isn't worth holding it out
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.
Don't worry about taking to yourself, it's late but I'm listening. :)
|
stopped merge... I have some issues with this |
assets/app/views/deployments.html
Outdated
| </div> | ||
| <div> | ||
| <dt>Replicas:</dt> | ||
| <dd>{{deployment.spec.replicas}}</dd> |
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.
if deployment.status.replicas is not equal to deployment.spec.replicas, should surface that somehow...
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.
Fixed to display exactly like in osc describe:
Replicas: {{deployment.status.replicas}} current / {{deployment.spec.replicas}} desired
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.
Later we can think about a more prominent warning icon in case they are different, or something like that.
|
restarting [merge], sorry about that... follow up with surfacing |
0ceec90 to
b7191df
Compare
|
rebuild bindata? |
b7191df to
0a04a7a
Compare
|
Done. |
|
Re[merge] |
|
Evaluated for origin up to 0a04a7a |
…fig_list Merged by openshift-bot
@liggitt one of my first contributions to the web console, could you give it a look to make sure it's sane?
@ironcladlou take a look at the screenshot, it's still WIP but how is it looking?
@smarterclayton fyi