Skip to content

Improve deploy --cancel output#4669

Merged
openshift-bot merged 3 commits intoopenshift:masterfrom
ironcladlou:better-cancel-message
Sep 15, 2015
Merged

Improve deploy --cancel output#4669
openshift-bot merged 3 commits intoopenshift:masterfrom
ironcladlou:better-cancel-message

Conversation

@ironcladlou
Copy link
Contributor

Make the deploy --cancel output more accurate and informative.

Fixes #4642

Make the deploy --cancel output more accurate and informative.
@ironcladlou
Copy link
Contributor Author

Example output:

$ oc deploy deploytester --cancel                                                                                                           
no deployments are in progress (latest deployment #1 failed about an hour ago)

$ oc deploy deploytester2 --cancel
there have been no deployments for test/deploytester2

$ oc deploy deploytester2 --cancel                                                                                                          
no deployments found to cancel

@ironcladlou
Copy link
Contributor Author

/cc @Kargakis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer moving formatRelativeTime probably under pkg/util/time or at least export it (we already import its package) and use that instead but you would have to change a couple of other places as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with @Kargakis and we decided that there needs to be a separate PR to extract various things here into pkg/util.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not into util, into util/*/ :)

@0xmichalis
Copy link
Contributor

LGTM, just nits

@ironcladlou
Copy link
Contributor Author

@smarterclayton should also take a look since he filed the original issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No deployment is in progress for deployment config foo (latest #3)

Or similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference in this particular case is there is no latest deployment. THere's nothing in progress, but there might be soon- we can't say for sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be more explicit. No deployments currently in progress for deployment
config foo (current #3)

On Tue, Sep 15, 2015 at 1:20 PM, Dan Mace notifications@github.com wrote:

In pkg/cmd/cli/cmd/deploy.go
#4669 (comment):

@@ -317,7 +322,18 @@ func (o *DeployOptions) cancel(config *deployapi.DeploymentConfig, out io.Writer
return fmt.Errorf("couldn't cancel deployment %s", strings.Join(failedCancellations, ", "))
}
if !anyCancelled {

  •   fmt.Fprintln(out, "no active deployments to cancel")
    
  •   if latest == nil {
    
  •       // TODO: this could mean that a new deployment is forthcoming but hasn't
    
  •       // yet been created; might not be worth trying to express in this
    
  •       // context.
    
  •       fmt.Fprintf(out, "no deployments are in progress\n")
    

The difference in this particular case is there is no latest deployment.
THere's nothing in progress, but there might be soon- we can't say for sure.


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4669/files#r39538283.

Clayton Coleman | Lead Engineer, OpenShift

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it right this time. Now it'll always show the latest deployment's status when there are >0 deployments.

@smarterclayton
Copy link
Contributor

Looks ok

@ironcladlou
Copy link
Contributor Author

Feedback addressed. [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5006/) (Image: devenv-fedora_2349)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6ef537c

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5006/)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6ef537c

openshift-bot pushed a commit that referenced this pull request Sep 15, 2015
@openshift-bot openshift-bot merged commit a834c5d into openshift:master Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants