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
Deploy, rollback, retry and cancel deployments from the web console #4177
Deploy, rollback, retry and cancel deployments from the web console #4177
Conversation
also fyi @smarterclayton, I want to address at least a couple things described in #3363. |
[test] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4874/) |
This is what I proposed for builds when I was looking at how to display logs. I assumed this would also apply to deployments. I used links for these actions so they would fit on the same line as the status. The secondary button style on the grey background blends in too much and looks like the button is disabled. If we do go with buttons for these actions we should use the primary (blue) button style or reconsider the grey background.
Is there a better way to phrase "Rollback to this"? It's long and an awkward phrase. "Redeploy"? "Restore"? |
I started with links, but the problem with them is that disabled is an important state for all these actions. When something is running for a given deployment config then every action except for cancel (Start Deployment, Retry, Rollback), on every deployment, has to be disabled. Although links can do it, it seems more visually intuitive with buttons. Let me try with the primary buttons to check how it looks.
I don't like "rollback to this" either, but I think the "rollback" term must be present since it's what we use to define that action, including in the cli. Simply "rollback" wouldn't denote what it really means, which is rolling back to a defined previous version. Maybe
Correct. By default we display them collapsed (first screenshot). I like the idea of making it clear in the status (notice we currently display it in the deployment name, like "frontend-3 (latest)", but it's not very clear). |
This isn't really something that I expect Fabiano to fix here, but I think the sub details here are getting really really hard to read (even the more minimal form from builds). I think we have a general information awareness problem here I'd like to see sorted out in 1.1. Remember, rollback creates a new version at the top, so also need to be able to link that action. I would try to be consistent with the CLI in output and behavior. |
@ajacobs21e another option, if we are really going to rely on buttons and they don't look good where they are, would be to have them aside with "view logs", in another color. |
Is this the correct / complete list of states and actions? I'm asking the visual designers on my team for input on how to make a disabled state button show up on a grey background. I didn't notice "Show older deployments" at first but now I see it in your first screenshot. I think it would be helpful to distinguish between the older deployments after they are expanded - perhaps a line below the current deployment? Another option could be to show the older deployments in a box with a white background, which would also solve the problem with the disabled buttons. edit: Disabled buttons should have some hover text to explain "disabled while deployment is running/pending" etc |
|
Yes, although if you look at the terminology used by others in the space On Mon, Aug 17, 2015 at 11:46 AM, Allie Jacobs notifications@github.com
Clayton Coleman | Lead Engineer, OpenShift |
"Revert" is a much nicer term |
That's correct, and there is also "New" which behaves exactly like "Pending" and "Running". ATM labels don't change when buttons are disabled.
Interesting, let me try that option. |
The UI of previous deployments should be similar to status, with an option On Mon, Aug 17, 2015 at 1:15 PM, Fabiano Franz notifications@github.com
Clayton Coleman | Lead Engineer, OpenShift |
@smarterclayton @ajacobs21e I like "Revert" too. |
@fabianofranz does "revert that deployment" mean something? To me ("new user") the two terms would mean the same thing (revert back to that deployment). Thinking about this more I would expect to see a "Revert" button associated with the name of the old deployment and not its status. @jwforres @smarterclayton does that make sense? Also if we are using buttons can the button be moved up a pixel so the text & button label are bottom aligned? |
Few changes: using "revert", active deployment in grey bg and older ones in white, button back to normal size and associated with the name (right alignment). Not yet addressing the information problems. |
Keep in mind revert creates a new deployment. It's a wrinkle that makes On Aug 17, 2015, at 1:30 PM, Allie Jacobs notifications@github.com wrote: @fabianofranz https://github.com/fabianofranz does "revert that Thinking about this more I would expect to see a "Revert" button associated Also if we are using buttons can the button be moved up a pixel so the text — |
@smarterclayton does "recreate" make more sense than "revert"? @fabianofranz I like the white background for older deployments. They should be labeled as such - either having text "Older deployments" or changing the 'Show older deployments' text to "Hide older deployments". Putting the button in the top right makes sense until we try to also put in the button for logs, metrics, and potentially any other extensions. Putting "view logs", "view metrics", "view X" under a dropdown would make sense but I think these actions should be separate. Here's a rough idea of one way to do this: |
I'm not a fan of dropdowns triggering actions when you select one of the items |
Having gone around the circle we should use roll back because it's On Aug 18, 2015, at 10:45 AM, Allie Jacobs notifications@github.com wrote: @smarterclayton https://github.com/smarterclayton does "recreate" make @fabianofranz https://github.com/fabianofranz I like the white background Putting the button in the top right makes sense until we try to also put in — |
Sounds good On Tue, Aug 18, 2015 at 11:31 AM, Clayton Coleman notifications@github.com
Allie Jacobs calendar |
5d63a08
to
3a1277b
Compare
Good ideas @fabianofranz. We have some options here.
Here are two other ideas I had: |
@ajacobs21e Yeah, the arguments about these action beings in the same level and non-destructive are correct, so both should be styled as primary as you said. "View" should not be a toggle, but a simple drop-down button (the vertical line after the label goes away). So, I like either one of your ideas or my option 2. I suggest we pick one and go with it for the scope of this pull request (notice that I'm not implementing logs here so there will be no "View" button for now, but I can leave it there commented for when we get logs and metrics). Which one would you suggest? |
@fabianofranz I would prefer to keep "Roll Back" "Cancel" etc near the status. Do you agree? If so then I prefer my 2nd option (with no status label). However that's a bigger change and would affect other similar pages - would the first one be better for now or is it ok to make a larger change now and roll it out to other pages later? @jwforres What do you think about using the status "Inactive" or "Complete (inactive)" for older deployments? |
@ajacobs21e I agree with them near the status, and I'd suggest we go with your first one for now (with the label). |
@fabianofranz cool, sounds good. Thanks for talking through this with me. |
return ['New', 'Pending', 'Running'].indexOf($scope.deploymentStatus(deployment)) > -1; | ||
}; | ||
|
||
$scope.describeDeploymentStatus = function(deployment) { |
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.
this should be in the view using the ng-switch directive, especially since you are only calling this once
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.
2b957ec
to
71558bc
Compare
}); | ||
return deploymentConfigDeploymentsInProgress; | ||
} | ||
|
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.
Wondering if we may want functions like this moved up into a service. It may be handy to have available in other contexts & keep the controllers light-weight. We don't have many services in front of DataService
yet, but this one might be a good conversation starter. DataService
provides methods like deployments.by("metadata.name")
, but prob shouldn't provide methods specific to a particular object. If we wrapped it we could then also have methods like deployments.associateRunningDeploymentToDeploymentConfig()
on the service. We might be able to shorten names then, perhaps something like deployments.byConfig(config)
.
Conceptually I'm thinking something like this:
// this would likely use the DataService.watch("replicationcontrollers", $scope, function(deployments) {});
replicationControllers.watch(context, function(deployments) {
});
// and the deployments object could look something like this, giving us data & functions:
return {
data: { /* the request data */ },
// functions we can share so they don't have to live in controllers
byConfig: function(config) { return /* */ },
foo: function() { /* */ }
}
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.
Yeah, it makes sense to me. I would handle it separately (not as part of this PR) but definitely makes sense.
71558bc
to
3f6ebb8
Compare
PR updated to remove the "Retry" option. Start, Cancel and Rollback are still available. |
05014c3
to
4ba69f6
Compare
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3268/) (Image: devenv-fedora_2306) |
2ffee32
to
78aea12
Compare
78aea12
to
446f5f6
Compare
446f5f6
to
130dc47
Compare
Evaluated for origin test up to 130dc47 |
[merge] |
Evaluated for origin merge up to 130dc47 |
…cks_from_console Merged by openshift-bot
This adds buttons to the "Deployments" page on the web console to "Start Deployment" (based on the latest config), "Retry" a failed deployment, "Cancel" a running one or "Rollback" to a previous successful deployment.
TODO: rollback with parameters like in the CLI.
Screenshots (1. collapsed view, 2. expanded view):