Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 15, 2015
Merged

Deployment configs on the web console, with deployments grouped #2178

merged 1 commit into from
May 15, 2015

Conversation

fabianofranz
Copy link
Member

@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

console-dc

liggitt
liggitt reviewed May 12, 2015
View reviewed changes
assets/app/views/deployments.html
</div>
</dl>

<dl class="dl-horizontal left indent">
Copy link
Contributor

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.

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch from f30c4e7 to ff4ebea Compare May 12, 2015 03:47
@fabianofranz
Copy link
Member Author

I just pushed a few improvements and a new screenshot that reflects them.

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch 2 times, most recently from 04c4422 to 42b11b1 Compare May 12, 2015 15:22
@liggitt
Copy link
Contributor

liggitt commented May 12, 2015

cc @spadgett

@spadgett
Copy link
Member

Fixes #2145

@spadgett
Copy link
Member

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.

@spadgett
Copy link
Member

Do your changes work with label filters?

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch from 42b11b1 to bd90f66 Compare May 12, 2015 20:44
@fabianofranz fabianofranz changed the title WIP: deployment configs on the web console, with deployments grouped Deployment configs on the web console, with deployments grouped May 12, 2015
@fabianofranz
Copy link
Member Author

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.

@fabianofranz fabianofranz mentioned this pull request May 12, 2015
Merged
@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch 3 times, most recently from 610d765 to 5a42299 Compare May 13, 2015 18:49
@fabianofranz
Copy link
Member Author

UP

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch 5 times, most recently from 3281408 to 1afbfd9 Compare May 14, 2015 17:56
@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch 2 times, most recently from 97eb7d8 to b32524e Compare May 14, 2015 18:30
@fabianofranz
Copy link
Member Author

Fixed recent annotation changes.

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch 2 times, most recently from 955b478 to 0ceec90 Compare May 15, 2015 03:17
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/1943/) (Image: devenv-fedora_1518)

liggitt
liggitt reviewed May 15, 2015
View reviewed changes
assets/app/views/deployments.html
<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}}
Copy link
Contributor

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?

Copy link
Contributor

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?)

Copy link
Contributor

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

Copy link
Member Author

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. :)

@liggitt
Copy link
Contributor

liggitt commented May 15, 2015

stopped merge... I have some issues with this

liggitt
liggitt reviewed May 15, 2015
View reviewed changes
assets/app/views/deployments.html
</div>
<div>
<dt>Replicas:</dt>
<dd>{{deployment.spec.replicas}}</dd>
Copy link
Contributor

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...

Copy link
Member Author

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

Copy link
Member Author

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.

@liggitt
Copy link
Contributor

liggitt commented May 15, 2015

restarting [merge], sorry about that... follow up with surfacing deployment.status.replicas please

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch from 0ceec90 to b7191df Compare May 15, 2015 04:12
@liggitt
Copy link
Contributor

liggitt commented May 15, 2015

rebuild bindata?

@fabianofranz fabianofranz force-pushed the 643_console_deployment_config_list branch from b7191df to 0a04a7a Compare May 15, 2015 04:20
@fabianofranz
Copy link
Member Author

Done.

@fabianofranz
Copy link
Member Author

Re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 0a04a7a

openshift-bot pushed a commit that referenced this pull request May 15, 2015
Merge pull request #2178 from fabianofranz/643_console_deployment_con…
1048b0e 
…fig_list

Merged by openshift-bot
@openshift-bot openshift-bot merged commit 1048b0e into openshift:master May 15, 2015
@spadgett spadgett mentioned this pull request May 15, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fabianofranz @liggitt @spadgett @openshift-bot @ironcladlou