Skip to content
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

[cli] Improve UX of graceful shutdown message #4064

Merged
merged 4 commits into from
Feb 13, 2017
Merged

[cli] Improve UX of graceful shutdown message #4064

merged 4 commits into from
Feb 13, 2017

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Feb 8, 2017

What does this PR do?

Improve UX of graceful shutdown message

  • base scripts : catch error code of action lifecycle and return it
  • cha-lib : return error code based on http error code
  • display better message when auth is required
  • skip graceful mode when repo is mounted

What issues does this PR fix or reference?

codenvy/codenvy#1682

Changelog

Improves UX of graceful shutdown message.

Release Notes

We have made improvements to the CLI graceful shutdown behavior to catch additional errors, communicate more clearly when authentication is required for stop and skip the graceful mode when a repo is mounted.

Docs PR

n/a (improvement on UX, command is the same)

Change-Id: I1ad021426444b5b4d880df518512e53ef4828336
Signed-off-by: Florent BENOIT [email protected]

@benoitf benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Feb 8, 2017
@benoitf benoitf self-assigned this Feb 8, 2017
Copy link

@bmicklea bmicklea left a comment

Choose a reason for hiding this comment

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

tweaked release note entry

@codenvy-ci
Copy link

Build # 1911 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1911/ to view the results.

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

I'm not an expert in neither modern CLI code nor TS code we use, so can't do real technical review.
As for what I understand it looks good.

@benoitf
Copy link
Contributor Author

benoitf commented Feb 9, 2017

@garagatyi I mentioned you if you wanted to check it was now working as you're expecting based the issue you reported.

- base scripts : catch error code of action lifecycle and return it
- cha-lib : return error code based on http error code
- display better message when auth is required
- skip graceful mode when repo is mounted

Change-Id: I1ad021426444b5b4d880df518512e53ef4828336
Signed-off-by: Florent BENOIT <[email protected]>
@@ -29,13 +29,23 @@ cmd_stop() {
FORCE_STOP=false
if [[ "$@" == *"--skip:graceful"* ]]; then
FORCE_STOP=true
elif local_repo; then
warning "Development mode [skip graceful stop]"

Choose a reason for hiding this comment

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

Why do you do this for local_repo, but not local_assembly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the issue ppl were asking for local repo mode
I'm also ok for local_assembly. I will add

cmd_lifecycle action "graceful-stop" "$@" >> "${LOGS}" 2>&1 || GRACEFUL_STATUS_RESULT=$?
# error on authentication (401 modulo 256 = 145)
if [[ ${GRACEFUL_STATUS_RESULT} -eq 145 ]]; then
error "Authentication failed on the system. Please provide --user and -password values or user --skip:graceful to bypass graceful stop."

Choose a reason for hiding this comment

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

"Authentication failed during graceful shutdown (hint: --skip:graceful to bypass)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TylerJewell in the issue it was requested to provide the argument for authentication (--user and --password) when auth is required

error "Authentication failed on the system. Please provide --user and -password values or user --skip:graceful to bypass graceful stop."
return 2;
elif [[ ${GRACEFUL_STATUS_RESULT} -ne 0 ]]; then
error "We encountered an error -- see $CHE_HOST_CONFIG/cli.log. Graceful stop can be skipped with --skip:graceful"

Choose a reason for hiding this comment

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

"We encountered an error during graceful stop - see $CHE_HOST_CONFIG/cli.log."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't we add the hint on the fact that graceful stop can be skipped ?

Choose a reason for hiding this comment

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

If you do it have it as (hint: --skip:graceful does not wait for workspace stop)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@codenvy-ci
Copy link

…ipts : catch error code of action lifecycle and return it - cha-lib : return error code based on http error code - display better message when auth is required - skip graceful mode when repo is mounted

Change-Id: I4e89605fd3c42ad6ba02115c32297d16f49b3ffe
Signed-off-by: Florent BENOIT <[email protected]>
…ipts : catch error code of action lifecycle and return it - cha-lib : return error code based on http error code - display better message when auth is required - skip graceful mode when repo is mounted

Change-Id: Icc0caeb4db3dc3a1be802de95bb1f1d505f93b40
Signed-off-by: Florent BENOIT <[email protected]>
@codenvy-ci
Copy link

@benoitf
Copy link
Contributor Author

benoitf commented Feb 13, 2017

@TylerJewell updated

@benoitf benoitf merged commit c5dc519 into master Feb 13, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 13, 2017
@benoitf benoitf added this to the 5.3.0 milestone Feb 13, 2017
@TylerJewell TylerJewell deleted the che#1682 branch February 15, 2017 00:01
@JamesDrummond JamesDrummond mentioned this pull request Feb 17, 2017
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
* Fix eclipse-che#1682 : Improve UX of graceful shutdown message
- base scripts : catch error code of action lifecycle and return it
- cha-lib : return error code based on http error code
- display better message when auth is required
- skip graceful mode when repo is mounted

Change-Id: I1ad021426444b5b4d880df518512e53ef4828336
Signed-off-by: Florent BENOIT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants