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: Use new che-mount & adds mechanism for CLI to update itself to new versions #2535

Merged
merged 15 commits into from
Sep 23, 2016

Conversation

TylerJewell
Copy link

@TylerJewell TylerJewell commented Sep 21, 2016

What does this PR do?

1: Alters the syntax so that volume mounts can have windows paths with spaces and dots.
2: Changes the syntax so that always $PWD will be selected as the local mount point.
3: Adds in discovery - if there is only a single ws running, will find the SSH port and use that.
4: If there are multiple workspaces with valid SSH ports, lists all of them and their ports.
5: Adds in new capability that allows users to download specific version of CLI and run it.
6: Uses curl to download initial CLI if it is not already there.

This new logic assumes that SSH:
1: Is running on port 22/tcp within the container, and:
2: Is accessible by the user named 'user'.

It will be better in the future, if we only exchange keys and automate this. Will wait for Florent's improvements here.

@TylerJewell TylerJewell added kind/enhancement A feature request - must adhere to the feature request template. team/pm labels Sep 21, 2016
@TylerJewell TylerJewell added this to the 5.0.0-M3 milestone Sep 21, 2016
@TylerJewell TylerJewell self-assigned this Sep 21, 2016
@TylerJewell
Copy link
Author

@benoitf @l0rd

@codenvy-ci
Copy link


if [ $CURRENT_WS_COUNT -eq 0 ]; then
error "${CHE_MINI_PRODUCT_NAME} mount: We could not find any running workspaces"

Copy link
Contributor

Choose a reason for hiding this comment

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

You should return after printing the error message

Copy link
Author

Choose a reason for hiding this comment

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

good catch

-v ${HOME}/.unison:${HOME}/.unison \
-v /etc/group:/etc/group:ro \
-v /etc/passwd:/etc/passwd:ro \
-u $(id -u ${USER})
Copy link
Contributor

Choose a reason for hiding this comment

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

The backslash at the end of the line is missing

Copy link
Author

Choose a reason for hiding this comment

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

yes, another good catch.

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

I've tested on OSX and Linux. There are couple of things to fixes. Besides that LGTM

-u $(id -u ${USER})
-v "${MOUNT_PATH}":/mnthost \
"${CHE_MOUNT_IMAGE_NAME}":"${CHE_UTILITY_VERSION}" \
"${GLOBAL_GET_DOCKER_HOST_IP}" $WS_PORT
Copy link
Contributor

Choose a reason for hiding this comment

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

indent is wrong

Copy link
Author

Choose a reason for hiding this comment

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

The indent was intended that way for readability - because it's the post container params.

-v "${HOME_PATH}"/.ssh:/root/.ssh \
-v "${MOUNT_PATH}":/mnthost \
"${CHE_MOUNT_IMAGE_NAME}":"${CHE_UTILITY_VERSION}" \
"${GLOBAL_GET_DOCKER_HOST_IP}" $WS_PORT
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

Copy link
Author

Choose a reason for hiding this comment

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

The indent was intended that way for readability - because it's the post container params.


elif [ $CURRENT_WS_COUNT -eq 1 ]; then
RUNNING_WS_PORT=$(docker inspect --format='{{ (index (index .NetworkSettings.Ports "22/tcp") 0).HostPort }}' ${CURRENT_WS_INSTANCES})
info "${CHE_MINI_PRODUCT_NAME} mount: Connecting to remote workspace on port $RUNNING_WS_PORT"
Copy link
Contributor

Choose a reason for hiding this comment

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

side note
if we're always using info "${CHE_MINI_PRODUCT_NAME}
"${CHE_MINI_PRODUCT_NAME} " should be inside info function

or we need to add another info commands to avoid to write ${CHE_MINI_PRODUCT_NAME} in all cals

Copy link
Author

Choose a reason for hiding this comment

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

Yes, i agree - will address in another improvement.

error "${CHE_MINI_PRODUCT_NAME} mount: We could not find any running workspaces"

elif [ $CURRENT_WS_COUNT -eq 1 ]; then
RUNNING_WS_PORT=$(docker inspect --format='{{ (index (index .NetworkSettings.Ports "22/tcp") 0).HostPort }}' ${CURRENT_WS_INSTANCES})
Copy link
Contributor

@benoitf benoitf Sep 22, 2016

Choose a reason for hiding this comment

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

what if there is no ssh running ?

Copy link
Author

Choose a reason for hiding this comment

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

I made a number of improvements to handle this:

  1. I now get a list of all ws containers.
  2. I added in a check to see if they have ssh or not.
  3. I then print out a list of all the containers their SSH port & whether they have SSH.
INFO: che mount: Setting local mount path to /c/codenvy/tmp/dir 4
INFO: che mount: Searching for running workspaces with open SSH port...
INFO: che mount: Re-run with 'che mount <ssh-port>'
WS CONTAINER ID    HAS SSH?    SSH PORT
3ca5f2d5c302       y           32797
33c0ce87fabf       n
679494b6cae7       y           32788

@codenvy-ci
Copy link

@TylerJewell TylerJewell changed the title CLI: Use new che-mount & discover ws CLI: Use new che-mount & adds mechanism for CLI to update itself to new versions Sep 23, 2016
@codenvy-ci
Copy link

@benoitf
Copy link
Contributor

benoitf commented Sep 23, 2016

cli.sh is being part of this pull request but it seems cli.sh is already in che repository which look strange ? as this PR seems to want to split it in two parts

@@ -0,0 +1,956 @@
init_global_variables() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires copyright headers + shebang

Copy link
Author

Choose a reason for hiding this comment

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

added.

if is_boot2docker && has_docker_for_windows_client; then
if [[ "${CHE_DATA_FOLDER,,}" != *"${USERPROFILE,,}"* ]]; then
CHE_DATA_FOLDER=$(get_mount_path "${USERPROFILE}/.${CHE_MINI_PRODUCT_NAME}/")
warning "Boot2docker for Windows - CHE_DATA_FOLDER set to $CHE_DATA_FOLDER"
Copy link
Contributor

Choose a reason for hiding this comment

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

indent look odd to me

Copy link
Author

Choose a reason for hiding this comment

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

fixed

usage
;;
esac
execute_cli "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

missing end of line

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@benoitf
Copy link
Contributor

benoitf commented Sep 23, 2016

$ ./che.sh start
INFO: Pulling image curl:latest
INFO: Downloading cli-latest
INFO: ECLIPSE CHE: Already have image codenvy/che-server:latest
INFO: ECLIPSE CHE: Starting container...
INFO: ECLIPSE CHE: Server logs at "docker logs -f che-server"
INFO: ECLIPSE CHE: Server booting...
INFO: ECLIPSE CHE: Booted and reachable
INFO: ECLIPSE CHE: http://localhost:8080

$ export CHE_VERSION=nightly
benoitf@MacBookFlorent:che (che-mount-cli=)$ ./che.sh start
INFO: ECLIPSE CHE: Already have image codenvy/che-server:nightly
INFO: ECLIPSE CHE: Starting container...
INFO: ECLIPSE CHE: Server logs at "docker logs -f che-server"
INFO: ECLIPSE CHE: Server booting...
INFO: ECLIPSE CHE: Booted and reachable
INFO: ECLIPSE CHE: http://localhost:8080

@TylerJewell TylerJewell merged commit c77e542 into master Sep 23, 2016
@TylerJewell TylerJewell deleted the che-mount-cli branch September 23, 2016 14:01
@codenvy-ci
Copy link

Build # 502 - FAILED

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

@bmicklea bmicklea mentioned this pull request Sep 28, 2016
63 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
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.

None yet

4 participants