-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/463/ |
|
||
if [ $CURRENT_WS_COUNT -eq 0 ]; then | ||
error "${CHE_MINI_PRODUCT_NAME} mount: We could not find any running workspaces" | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, another good catch.
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent is wrong
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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:
- I now get a list of all ws containers.
- I added in a check to see if they have ssh or not.
- 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
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/486/ |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/488/ |
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires copyright headers + shebang
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing end of line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
653cda9
to
0b68664
Compare
0b68664
to
653cda9
Compare
Build # 502 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/502/ to view the results. |
…ew versions (eclipse-che#2535) * initial * update usage
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.