Add support to list deployment history #2128
Conversation
fabianofranz
force-pushed
the
614_deployment_history
branch
from
e243771 to
136eac8
Compare
| fmt.Fprintf(out, "Latest deployment for %s/%s:\n", namespace, name) | ||
| printLines(out, indent, 1, d.describeDeployment(deploy.(*graph.DeploymentConfigNode))...) | ||
| node := deploy.(*graph.DeploymentConfigNode) | ||
| deploymentsCount := len(node.Deployments) + 1 |
There was a problem hiding this comment.
@ironcladlou I need to know the number of deployments of a dc, is this correct? len(node.Deployments) + 1 (the active deployment)
There was a problem hiding this comment.
@fabianofranz I haven't yet digested all the graph stuff going on here, but I can't see yet why you'd need to add 1- there's no distinction between an "active" deployment and any other in the system, so if you list RCs filtered by the deploymentConfig label, you should get an accurate count...
|
@ironcladlou @smarterclayton please review. |
fabianofranz
force-pushed
the
614_deployment_history
branch
from
136eac8 to
185d762
Compare
| @@ -26,6 +26,12 @@ NOTE: This command is still under active development and is subject to change.` | |||
| deploy_example = ` // Display the latest deployment for the 'database' deployment config | |||
| $ %[1]s deploy database | |||
|
|
|||
| // List all deployments for the 'database' deployment config | |||
| $ %[1]s deploy database -L | |||
There was a problem hiding this comment.
So why wouldn't I just describe dc/database? What does this add that the other does not?
There was a problem hiding this comment.
This one will list the history of deployments for the given dc, while describe only gives you the latest one. Also extends the functionality that was already available in the command when running without flags. Sample output:
All 6 deployments for test1/database:
#6 deployed about an hour ago
#5 deployed about an hour ago
#4 deployed about an hour ago
#3 deployed 2 hours ago
#2 deployed 2 hours ago
#1 deployed 2 hours ago
I do think though we could add more information to the output (or/and add a --verbose mode). What would you guys suggest?
There was a problem hiding this comment.
On May 7, 2015, at 8:19 PM, Fabiano Franz [email protected] wrote:
In pkg/cmd/cli/cmd/deploy.go:
@@ -26,6 +26,12 @@ NOTE: This command is still under active development and is subject to change.
deploy_example =// Display the latest deployment for the 'database' deployment config
$ %[1]s deploy database
- // List all deployments for the 'database' deployment config
- $ %[1]s deploy database -L
This one will list the history of deployments for the given dc, while describe only gives you the latest one. Also extends the functionality that was already available in the command when running without flags. Sample output:Why doesn't describe give me that? Why can't describe show me historical data? Why do we need two?
All 6 deployments for test1/database:#6 deployed about an hour ago
#5 deployed about an hour ago
#4 deployed about an hour ago
#3 deployed 2 hours ago
#2 deployed 2 hours ago
#1 deployed 2 hours ago
I do think though we could add more information to the output (or/and add a --verbose mode). What would you guys suggest?—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
Describe could give you that, for sure.
Still, I do like this idea of having it somehow attached to the deploy command, because user-friendliness (the flow of doing osc deploy database --retry then osc deploy database -L seemed fluid). Also, displaying the complete deployment history in describe could become too much depending on the history size, and given describe doesn't provide an argument to control it (limit the history size, etc).
We could do a mid-term, where we add a few more items from the history to describe, but still limit to say 3 or 5, and add more information to this one, similar to what describe does (maybe just a little less verbose, say not displaying the pod status and things like that). How does it sound?
There was a problem hiding this comment.
Or maybe describe display the latest 3 in full details, and all others in one line each, like this command is doing atm.
There was a problem hiding this comment.
I just don't know why we would want to replicate describe somewhere else. It's our go to for "get info about this". This command is about action and maybe a bit of status.
On May 7, 2015, at 9:23 PM, Fabiano Franz [email protected] wrote:
In pkg/cmd/cli/cmd/deploy.go:
@@ -26,6 +26,12 @@ NOTE: This command is still under active development and is subject to change.
deploy_example =// Display the latest deployment for the 'database' deployment config
$ %[1]s deploy database
- // List all deployments for the 'database' deployment config
- $ %[1]s deploy database -L
Or maybe describe display the latest 3 in full details, and all others in one line each, like this command is doing atm.—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
Ok, let me cook something around describe. What do you think about displaying the complete history, but only the latest 3 deployments in history in full details (like the latest one is displayed today), and all others summarized? Or let's just display the full history with details (I didn't check yet about performance, not sure if every deployment would require an additional api call, etc)?
fabianofranz
force-pushed
the
614_deployment_history
branch
from
185d762 to
ef48deb
Compare
|
@smarterclayton @ironcladlou Added deployments history to and |
|
Why have -L? Just display the last three on osc deploy automatically in simple mode.
|
|
I guess it's less scriptable. Still, we have osc status and osc describe DC, I don't think we have a need for -L
|
|
Ok, I'll remove the existing logic around The main reason for having a "short" version of the older deployments in the Other comments? |
|
I don't even know that you need more than the last 2 deployment. You care about the current and it's state, and the pods that are running on any older ones. Deployment Configs should eventually have a status field updated with the state of their rcs, like rcs and pods. Then we wouldn't need the queries.
|
fabianofranz
force-pushed
the
614_deployment_history
branch
from
ef48deb to
2761945
Compare
|
Removed the |
fabianofranz
force-pushed
the
614_deployment_history
branch
from
b647a12 to
41b7c88
Compare
|
I don't like the layout of the single deployment line for the deploy command. We're taking five lines to convey what can be done in one. Should be If there's a failure, show back to the last successful. I don't think the describe line helps. |
fabianofranz
force-pushed
the
614_deployment_history
branch
from
41b7c88 to
f786fa0
Compare
|
@smarterclayton Fixed, using just one line now. Showing back to the last successful in case there's a failure or deployment pending. Examples: or |
|
looks good. |
fabianofranz
force-pushed
the
614_deployment_history
branch
from
f786fa0 to
af07093
Compare
|
Squashed, [merge]. |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2149/) (Image: devenv-fedora_1506) |
|
[Test]ing while waiting on the merge queue |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2149/) |
fabianofranz
force-pushed
the
614_deployment_history
branch
from
af07093 to
b217ed6
Compare
|
Evaluated for origin up to b217ed6 |
Merged by openshift-bot
Makes use of the existing
osc deploycommand to list the deployments history through the-Lflag. Loosely inspired bygit logand some ideas fromgit diff --stat.Already available in
master:Added by this PR:
Other features and flags remain the same.
The UPSTREAM commit is under discussion here: spf13/pflag#18.