Refactor CLI: Faster boot, add `--help` global option, standardize command lifecycle #4006

TylerJewell · 2017-02-02T19:31:37Z

What does this PR do?

This PR refactors eclipse/che-base so that script files are loaded in an ordered version. Utility functions are only included within the files that need them, and those files are loaded in sequence. The sequence in which files are loaded are ordered by number with the library.sh loaded last, followed by the appropriate command files. Also, 6-8 functions that were not longer being used were removed.

Also adds in a refactoring of how command files are loaded so that there is a standard lifecycle for how commands are loaded, hooks invoked, and body of the command is invoked. Now, all commands must be in a single file that reflects the name of the command with a cmd_<name>.sh file format. In that file, you can optionally have a pre_cmd_<name>() and post_cmd_<name>() functions which will be called before and after the command. Using this functionality, we have added a --help global option which then presents the command-specific help for that command.

Additionally, found that we were not including docker/compose:1.8.1 in our list of images in the /version folder. This particular improvement includes this image in the /nightly folder. @riuvshin - Added the appropriate compose image line entry to the version manifests of 5.3.0, latest, and nightly. These changes now require IMAGE_COMPOSE=docker/compose:1.8.1 to be added to all versions, as this is now what is referenced in the CLI.

We have seen some challenges with people upgrading to new versions of the CLI when they are using nightly and also mounting :/repo. We have now added two additional checks to avoid this problem. First, the base and the CLI both have an API version constant that must match. This is loaded as the first check and this check will fail if developers somehow work with a base image that does not match CLI version. Or if base version has been incremented and CLI version not properly recompiled to match.

Second, we have added a check that if you mount :/repo, we will compare the base version inside the image with the version that is in the same file in the repo. If these do not match we print a warning as it means that your branch has scripts that are not compatible with the Docker image that you loaded.

This also fixes a small bug with running tests for Windows.

We have also changed --force to --skip:graceful to solve a problem where --force is designed to mean forcible removal of images. This was a design flaw.

Fixes an issue with Docker compose services not inheriting tailored container names and some flaws in the logic around checking for booted containers if there are multiple services running.

Adapt Other CLI Assemblies

Place post_init() method into post_init.sh script file
Place pre_init() method into pre_init.sh script file
Change UTILITY_IMAGE_CHEIP to BOOTSTRAP_IMAGE_CHEIP
Add CHE_CLI_API_VERSION=1 to the pre_init.sh file
Rename cli.sh to be cmd_extension.sh for proper loading
Add --help option to display help for every command

For the Codenvy adaptation, had to make sure that all constants were moved into the pre_init method. This was designed to ensure backward compatibility with certain variable names.

Codenvy: codenvy/codenvy#1701
Codenvy SAAS: https://github.com/codenvy/customer-saas/pull/133

What issues does this PR fix or reference?

codenvy/codenvy#1681 (comment)

Changelog

CLI - if :/repo mounted, check CHE_BASE_API_VERSION in repo to match version in image
Change --force flag to --skip:graceful in cmd_stop()
Add API version check between CLI and base image to immediately fail if out of sync
Added Docker compose to image registry
Fix mapping of custom container names to docker compose services
Add --help global option to display help for a specific command

Release Notes

N/A

Docs PR

N/A

riuvshin · 2017-02-02T19:48:03Z

as I understand this does not help because same version 5.3.0-SNAPSHOT maybe not compatible with some changes, because it is snapshots.
Example
I;ve installed nightly today it is 5.3.0-SNAPSHOT.
later today some one added some new very cool feature to the CLI
tomorrow I run my CLI again and it is self pulled (that is how it works)
and as result due to mounted repo with my dev branch does not have new CLI changes
and CLI will fail and version still the same 5.3.0-SNAPSHOT

I think we should not bound nightly to versions that is wrong we will never connect the dots. it is possible only if we tag and fix everything like we do for tags. (maybe im wrong but that is what I think)

TylerJewell · 2017-02-02T19:49:14Z

We already tag and bound everything as best as we can. The taging and digests would not have solved the problem here.

TylerJewell · 2017-02-02T19:50:48Z

The issue that you describe can occur. But the issue would only occur for a very small number of files. There are 4 files that eclipse/che-base loads as helper files before :/repo is mounted. So, if we mount :/repo, we could perform a diff check of the four script files that were loaded to see if they different from what is in the repository, and hten error out.

riuvshin · 2017-02-02T19:56:48Z

what if we will remove mount of CLI files in dev mode ? that gives more pain then gain. I thought you are trying to fix issue with whic almost all devs in troubles now today I saw usecase that i;ve described 5-6 times and guys asked me for help, because in their branches CLI is obsolete and they should merge master to their branches which is really hard sometimes.
So my proposal is to remove dev mode for CLI, just not mount CLI files to CLI container and that's it!
that won't make CLI devs life harder because it is easy to rebuild images.
if CLI will be pulled tomorrow I don't care it will came working. @TylerJewell @benoitf @garagatyi WDYT guys?

TylerJewell · 2017-02-02T20:00:57Z

I don't believe that solves the problem - or necessarily increases the pain.

But I think maybe the partial problem is that only a partial set of the CLI files are mounted when you have :/repo mounted. Turns out that only the shell scripts in the commands/ are being loaded in this situation and then already the other files like library.sh are still using those that are in the image.

Maybe it's good enough just to fix this so that all files from the repository are loaded? The only files that would not be loaded in this case, are the files necessary for loading docker and detecting :/repo mount, which would be a small set that rarely change.

codenvy-ci · 2017-02-02T20:25:08Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1863/

benoitf · 2017-02-02T20:49:08Z

I think it won't work for artik as we have different version schemes

Signed-off-by: Tyler Jewell <[email protected]>

codenvy-ci · 2017-02-02T23:05:54Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1864/

riuvshin · 2017-02-03T10:34:02Z

agree with @benoitf, CLI should not know about project version. CLI should know only images tags.

TylerJewell · 2017-02-03T13:15:15Z

Florent now proposes that we have CLI version flags that we increment in the base and each derivative cli. We will compare their usage of the API version to ensure a perfect match. If we modify any of the core function behavior in the base we will increment this value and other CLIs that have not been recompile's and using repo mounts will interrupt with clear message.

Signed-off-by: Tyler Jewell <[email protected]>

benoitf · 2017-02-03T16:01:00Z

dockerfiles/base/scripts/base/commands/cmd_start.sh

-    info "Waiting for graceful stop of services..."
-    cmd_action "graceful-stop" "$@"
+    info "stop" "Stopping workspaces..."
+    cmd_action "graceful-stop" "$@" >> "${LOGS}" 2>&1 || true


if graceful stop fail, it would mean that we continue the stop command? I'm not sure it's what we should do if it's the case
if graceful fail, user need to figure out what he wants to do. Either he performs a force or he tries again the graceful stop

I was thinking that if there is a graceful stop fail, then we just turn it into a non-graceful stop :). But I see your point. You think it should be || false and then put it into an if statement?

or just fail, the action would have reported the error
BTW I don't think we should hide report of graceful stop by default as it may take a while you may want to know what is performed (as in any long task)
--silent mode could do only the log but I think I would be interested in how many workspaces have been stopped and how many I need

codenvy-ci · 2017-02-03T16:11:53Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1870/

Signed-off-by: Tyler Jewell <[email protected]>

codenvy-ci · 2017-02-03T17:33:14Z

Build # 1872 - FAILED

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

Signed-off-by: Tyler Jewell <[email protected]>

codenvy-ci · 2017-02-04T17:06:04Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1873/

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell · 2017-02-04T18:21:19Z

@benoitf - I think we are ready for final review. I adapted Codenvy and Codenvy-SAAS. This needs to be adapted for ARTIK IDE, which I will let you do. The instructions of what should be done should be fairly clear. This makes extending a custom assembly simpler.

codenvy-ci · 2017-02-04T19:27:49Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1875/

Signed-off-by: Tyler Jewell <[email protected]>

…envy-1681

Signed-off-by: Tyler Jewell <[email protected]>

codenvy-ci · 2017-02-05T19:48:15Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1878/

benoitf · 2017-02-06T08:44:07Z

dockerfiles/base/scripts/base/commands/cmd_action.sh

+  docker_run -it ${UTILITY_IMAGE_CHEACTION} "$@"
+}
+
+pre_cmd_action() {


if we call utility image che action with --help it displays the help automatically so we shouldn't write again the help there as it is duplicating the help (and then we will have errors of data being not consistent)
we should only delegate

I thought about that but had problems with delegation. The output was not controllable and some of the explanations did not make sense. I was worried about our if syncness like you said but could not figure out a clean way to present the information.

benoitf · 2017-02-06T08:46:25Z

dockerfiles/base/scripts/base/commands/cmd_destroy.sh

@@ -75,3 +75,18 @@ directory_is_empty() {
    return 0
  fi
 }
+
+pre_cmd_destroy() {
+  if get_command_help; then


do we need to define help inside a pre_'command' ? with a check on if get_command_help

we could have commands named cmd_destroy_help or help_cmd_destroy and in the execution of the lifecycle, it the help command is invoked then it invokes the help command

it will simplify the code really need for the init stuff vs help stuff

Yes, this was a good idea. Should have thought of tihs before. Updated.

benoitf · 2017-02-06T08:48:28Z

dockerfiles/base/scripts/base/commands/cmd_info.sh

+  info "--------   CONNECTIVITY TEST   --------"
+  info "---------------------------------------"
+
+  info "network" "eclipse/che-ip: ${GLOBAL_HOST_IP}"


we should replace "eclipse/che-ip" by the utility name
I'm also unsure if we really use ${GLOBAL_HOST_IP} (IMHO we are using ${CHE_HOST})

Replaced the text with image name and CHE_HOST.

benoitf · 2017-02-06T08:49:45Z

dockerfiles/base/scripts/base/commands/cmd_restart.sh

+  info "restart" "Restarting..."
+  cmd_lifecycle stop ${@}
+
+  # Need to remove any stop parameters from the command line otherhwise the start will fail


small typo on otherwise

yep... fixed

benoitf · 2017-02-06T08:54:00Z

dockerfiles/base/scripts/base/commands/cmd_stop.sh

+  if server_is_booted $(get_server_container_id $CHE_CONTAINER_NAME); then 
+    if [[ ${FORCE_STOP} = "false" ]]; then
+      info "stop" "Stopping workspaces..."
+      if ! $(cmd_lifecycle action "graceful-stop" "$@" >> "${LOGS}" 2>&1 || false); then


if we drop output of the graceful-stop we won't see the upcoming error message saying that authentication needs to be used. I would expect that it's directly printed on the screen without any user action on having to check cli.log (also ppl may not know directly where the file is located by using only cli.log)

I am not a fan of delegating output to utilities as the format changes and is inconsistent. Is this a check we can do in the cli to capture that message?

The format can be consistent. It's just that it is changing for now a lot.
We could show raw output in case of error. BTW if there is an error user might need to see the output. So it's like a shortcut.

We can also return internal error code of REST API call to the cli
0 = ok
409 : auth problem, etc so in cli we can say that there is an auth issue, etc

benoitf · 2017-02-06T08:54:40Z

dockerfiles/base/scripts/base/commands/cmd_sync.sh

  if [[ "${SYNC_MOUNT}" = "not set" ]]; then
    info "Welcome to $CHE_FORMAL_PRODUCT_NAME!"
    info ""
    info "We could not detect a location to do the sync."
    info "Volume mount a local directory to ':/sync'."
    info ""
-    info "  docker run .... -v <YOUR_LOCAL_SYNC_PATH>:/sync ...."
+    info "  docker run ... -v <YOUR_LOCAL_SYNC_PATH>:/sync ...."


you may drop one dot in the right part of the line as well

benoitf · 2017-02-06T08:56:02Z

dockerfiles/base/scripts/base/library.sh

+  docker rm -f fakeagent >> "${LOGS}"  
+}
+
+test1() {


should we drop some of code of the network info command as it seems we have a lot of duplicates
also the function name test1() is hard to know what is really done there and for which part this test is.

This has been here for a few commits so we can try to address it in another pr for when we do an update around diagnostics.

benoitf · 2017-02-06T09:01:30Z

dockerfiles/base/scripts/base/startup_05_pre_exec.sh

+  if [ -n "$(type -t $POST_COMMAND)" ] && [ "$(type -t $POST_COMMAND)" = function ]; then
+    eval $POST_COMMAND "$@"
+  fi
+}


missing newline

Signed-off-by: Tyler Jewell <[email protected]>

codenvy-ci · 2017-02-06T17:15:41Z

Build # 1889 - FAILED

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

…mmand lifecycle (eclipse-che#4006) Signed-off-by: Tyler Jewell <[email protected]> * multi container fixup * Update images * add --help parameter * move entrypoint * remove empty

repo compare

59fea14

refactoring

93266c3

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell changed the title ~~Fix for codenvy-1681 - compare Che base CLI version to :/repo~~ Refactor CLI scripts to load minimum functions in sequence Feb 2, 2017

TylerJewell added the kind/bug Outline of a bug - must adhere to the bug report template. label Feb 2, 2017

fixup

2f729d5

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell mentioned this pull request Feb 3, 2017

CLI restart re-pulls all the layers of all the images of Codenvy and even ignores --fast codenvy/codenvy#1690

Closed

fixup

c1dc865

benoitf reviewed Feb 3, 2017

View reviewed changes

Tyler Jewell added 2 commits February 3, 2017 08:34

fixup

f53339f

Signed-off-by: Tyler Jewell <[email protected]>

fixup

dfbca91

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell mentioned this pull request Feb 4, 2017

CHE-3359 - Starting a workspace as an user #3376

Closed

multi container fixup

da0e27f

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell mentioned this pull request Feb 4, 2017

Adapt CLI for refactoring of CHE-4006 codenvy/codenvy#1701

Merged

Tyler Jewell added 4 commits February 4, 2017 09:19

fixup

d945319

fixup

5c6ea2f

Signed-off-by: Tyler Jewell <[email protected]>

fixup

a063f13

Signed-off-by: Tyler Jewell <[email protected]>

fixup

a5da0da

Signed-off-by: Tyler Jewell <[email protected]>

Update images

0a9732c

Tyler Jewell added 2 commits February 5, 2017 09:09

add --help parameter

ea4d821

Signed-off-by: Tyler Jewell <[email protected]>

Merge branch 'codenvy-1681' of http://github.com/eclipse/che into cod…

71b4d5d

…envy-1681

TylerJewell changed the title ~~Refactor CLI scripts to load minimum functions in sequence~~ Refactor CLI: Faster boot, add --help global option, standardize command lifecycle Feb 5, 2017

TylerJewell added the kind/enhancement A feature request - must adhere to the feature request template. label Feb 5, 2017

Tyler Jewell added 2 commits February 5, 2017 09:36

fixup

0c588c0

Signed-off-by: Tyler Jewell <[email protected]>

fixup

ceae714

Signed-off-by: Tyler Jewell <[email protected]>

benoitf reviewed Feb 6, 2017

View reviewed changes

Tyler Jewell added 3 commits February 6, 2017 06:02

refactor

d03567c

fixup

349377e

Signed-off-by: Tyler Jewell <[email protected]>

move entrypoint

f51894a

Signed-off-by: Tyler Jewell <[email protected]>

benoitf approved these changes Feb 6, 2017

View reviewed changes

remove empty

ee7393a

Signed-off-by: Tyler Jewell <[email protected]>

riuvshin approved these changes Feb 6, 2017

View reviewed changes

fixup

6be257c

Signed-off-by: Tyler Jewell <[email protected]>

TylerJewell added this to the 5.3.0 milestone Feb 6, 2017

TylerJewell merged commit 8287fb6 into master Feb 6, 2017

TylerJewell deleted the codenvy-1681 branch February 6, 2017 15:37

TylerJewell mentioned this pull request Feb 13, 2017

Cannot start multiple Eclipse Che containers on the same server due (maybe) to ports overlapping. #4130

Closed

JamesDrummond mentioned this pull request Feb 17, 2017

5.3.0 Milestone Overview #4030

Closed

9 tasks

Refactor CLI: Faster boot, add --help global option, standardize command lifecycle #4006

Refactor CLI: Faster boot, add --help global option, standardize command lifecycle #4006

Conversation

TylerJewell commented Feb 2, 2017 • edited Loading

What does this PR do?

Adapt Other CLI Assemblies

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

riuvshin commented Feb 2, 2017

TylerJewell commented Feb 2, 2017

TylerJewell commented Feb 2, 2017

riuvshin commented Feb 2, 2017 • edited Loading

TylerJewell commented Feb 2, 2017 • edited Loading

codenvy-ci commented Feb 2, 2017

benoitf commented Feb 2, 2017

codenvy-ci commented Feb 2, 2017

riuvshin commented Feb 3, 2017

TylerJewell commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Feb 3, 2017

codenvy-ci commented Feb 3, 2017

codenvy-ci commented Feb 4, 2017

TylerJewell commented Feb 4, 2017

codenvy-ci commented Feb 4, 2017

codenvy-ci commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Feb 6, 2017

Refactor CLI: Faster boot, add `--help` global option, standardize command lifecycle #4006

Refactor CLI: Faster boot, add `--help` global option, standardize command lifecycle #4006

TylerJewell commented Feb 2, 2017 •

edited

Loading

riuvshin commented Feb 2, 2017 •

edited

Loading

TylerJewell commented Feb 2, 2017 •

edited

Loading

benoitf Feb 6, 2017 •

edited

Loading