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

Add integration with vnc console #854

Closed
wants to merge 3 commits into from
Closed

Conversation

kozloffsky
Copy link

@kozloffsky kozloffsky commented Nov 8, 2018

Add integration with VncConsole

fixes #490

@ovirt-infra
Copy link

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

  1. Type ci test please on this PR to trigger automated tests for it.
  2. Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically.
  3. If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

@gregsheremeta
Copy link
Contributor

ci add to whitelist

@@ -35,4 +35,5 @@ module.exports = {
appNodeModules: resolveApp('node_modules'),
ownNodeModules: resolveApp('node_modules'),
nodePaths: nodePaths,
novmc: resolveApp('node_modules/@novnc/novnc')
Copy link
Contributor

Choose a reason for hiding this comment

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

change to:

novnc: resolveApp('node_modules/@novnc/novnc')

('n' vs 'm')

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace, please, 'm' with 'n'

@@ -35,4 +35,5 @@ module.exports = {
appNodeModules: resolveApp('node_modules'),
ownNodeModules: resolveApp('node_modules'),
nodePaths: nodePaths,
novmc: resolveApp('node_modules/@novnc/novnc')
Copy link
Contributor

@bond95 bond95 Nov 12, 2018

Choose a reason for hiding this comment

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

Is this module need? Cause I didn't see in https://github.com/patternfly/patternfly-react/tree/master/packages/patternfly-3/react-console @novnc/novnc requirement.

Copy link
Contributor

@bond95 bond95 Nov 12, 2018

Choose a reason for hiding this comment

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

Ok, I found it, but it's already in module dependencies, thus I little bit change question. Is this module need to install explicitly?

Copy link
Author

Choose a reason for hiding this comment

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

@bond95 This module does not have in dist regular js code so it needs to me babelized.

src/App.js Outdated
@@ -51,7 +51,9 @@ const App = ({ history, config, appReady }) => {
return (
<ConnectedRouter history={history}>
<div id='app-container'>
<VncConsole host='localhost' port='3333' />
<VncConsole encrypt='true'
path='eyJzYWx0IjoicDNXeWRiNUswWm89IiwiZGF0YSI6IiU3QiUyMnBvcnQlMjIlM0ElMjI1OTAzJTIyJTJDJTIyaG9zdCUyMiUzQSUyMjEwLjM3LjE2MC41MSUyMiUyQyUyMnNzbF90YXJnZXQlMjIlM0FmYWxzZSU3RCIsInNpZ25hdHVyZSI6IjNaWHgzS0VpL1gyMEVpNXVHekpueENxbTRIcUlHV051Wk5KeUYwa29WbHBOUXRiWVFWeHUrY0VuSHNlM29YN1AwVzBlTWhZVTU5NlFDK01HNldHT0NPYUJURzU0TFNpOEtqL0t6dW9WODN5MVZVM0dUbU5CNDlCdE1pQ3VqYm5qaHNlcTdDdGJ2S0tsV3hZRGhPSkRQSEhHbkpSR3JGNzluSXNsU2JTM2hNc2xyUUJqQ0crMXN5bkI1UklWODJJSTA0TElvVjAxYTMwRi95S3hoZmFJRFhuTEc3aVVFNWV5T09RZWFPbjZXWjFRMlN6ZWtUbFpOZEhndlhpYkNNbjN6VXVGZ2RLWWpUNWhKZmdRa3VNNGFaQTk3Q3U2bDBRY0JZUitGbDZvYm5pTGJFMWNMbVR3ekR4Qm12K2s3L2hzNGNvc2Q1eUJDOHkxd3VpTzMwWlRFZz09IiwiZGlnZXN0Ijoic2hhMSIsImNlcnRpZmljYXRlIjoiLS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tXG5NSUlGQ0RDQ0EvQ2dBd0lCQWdJQ0VBRXdEUVlKS29aSWh2Y05BUUVMQlFBd1p6RUxNQWtHQTFVRUJoTUNWVk14SkRBaUJnTlZCQW9NXHJcbkczSm9aWFl1YkdGaUxtVnVaeTVpY25FdWNtVmthR0YwTG1OdmJURXlNREFHQTFVRUF3d3BZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxcclxuYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB1TkRjeE5qZ3dIaGNOTVRnd09UQTJNRGcxTmpVeVdoY05Nak13T0RFeU1EZzFOalV5V2pCaFxyXG5NUXN3Q1FZRFZRUUdFd0pWVXpFa01DSUdBMVVFQ2d3YmNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dE1Td3dLZ1lEXHJcblZRUUREQ05pY25FdFpHVjJMbkpvWlhZdWJHRmlMbVZ1Wnk1aWNuRXVjbVZrYUdGMExtTnZiVENDQVNJd0RRWUpLb1pJaHZjTkFRRUJcclxuQlFBRGdnRVBBRENDQVFvQ2dnRUJBT0ZFZFRUUGxmTzdUZGwwRlY4TGFWMHRTQjJqcUVmeVFmd0k1clEyTkR1VElERHVOOFh2R0xIZ1xyXG4rV2tWQnJ6aDFicTJMcW9QcUJUWkJaejhXZCtoU2FKU0MvRGZ6RWlXbVhtbTUxdWd4RFo3N0tVODV6dndoU2x5Z1hocnEzMERSSVdKXHJcbk4rdEpQUDRXSXJyUjBKQTFkbFZaSThHb1liT2hVcTEwNDBBVHJDU0tPQWdiTXBSR3F4SkhhejNnZ3FaUzliYyttajV2Y2tDbzh3eHhcclxudjllNUJpU1RZQ1dURUU4T3JvTHdndjZjZVp4YU9RQ2NPRjNSOXJhY0QvRlhmSDFYellvTWNWbUhnakNrZVlZdjBGblMxRHNWcjBOQlxyXG5sb0Z1eTBTa1FGajhxZnludjFmS0lIL0VNQTVYNGNOalpDNmNTVUpxWENFNjd4ZUMwU2JPK1pmTHkxRUNBd0VBQWFPQ0FjSXdnZ0crXHJcbk1CMEdBMVVkRGdRV0JCUTJzajc0d3RGaUdmTDVjZytmdTBEczIrcEg3ekNCbWdZSUt3WUJCUVVIQVFFRWdZMHdnWW93Z1ljR0NDc0dcclxuQVFVRkJ6QUNobnRvZEhSd09pOHZZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxibWN1WW5KeExuSmxaR2hoZEM1amIyMDZPREF2YjNacFxyXG5jblF0Wlc1bmFXNWxMM05sY25acFkyVnpMM0JyYVMxeVpYTnZkWEpqWlQ5eVpYTnZkWEpqWlQxallTMWpaWEowYVdacFkyRjBaU1ptXHJcbmIzSnRZWFE5V0RVd09TMVFSVTB0UTBFd2daSUdBMVVkSXdTQmlqQ0JoNEFVVmllZnFjK2djSFVBQ2dDRThsVVFRQUxTTldHaGE2UnBcclxuTUdjeEN6QUpCZ05WQkFZVEFsVlRNU1F3SWdZRFZRUUtEQnR5YUdWMkxteGhZaTVsYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB4TWpBd1xyXG5CZ05WQkFNTUtXSnljUzFrWlhZdWNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dExqUTNNVFk0Z2dJUUFEQUpCZ05WXHJcbkhSTUVBakFBTUE0R0ExVWREd0VCL3dRRUF3SUZvREFnQmdOVkhTVUJBZjhFRmpBVUJnZ3JCZ0VGQlFjREFRWUlLd1lCQlFVSEF3SXdcclxuTGdZRFZSMFJCQ2N3SllJalluSnhMV1JsZGk1eWFHVjJMbXhoWWk1bGJtY3VZbkp4TG5KbFpHaGhkQzVqYjIwd0RRWUpLb1pJaHZjTlxyXG5BUUVMQlFBRGdnRUJBRWI1V1NvU3BPd1JUZjdVNnlPLzdwNUIvT3RiZHQrdGtPei9WSFBhRm54YlcwTUMrR3hGdEI0dVBQNVNwVHNQXHJcblZkWDJ0aVRVeXlKU3RMbXRMVWxlTHYrM0ZVbW9NcDV2eis4UnU0REE4QzNzdlhrYkNTZS9Qd2JOOEFyYlN2eXI5aUMveUpmN25OQ2Rcclxuckh1bzEzWnpZNDEvdTlUSHhiSW5pNHZPT00xTGw0WDBwOVRSTko1bzBhSTl5djBoR2NTV21HT0QyUi9pNWk1SkJFMVFFaVVrVlJWUVxyXG5vS3l5QXVveExtNkQ4bkJKMWRrUzhHVUtuM3J4blFFUGNZd2I0dGdaS3V6bG03RkdMOWI3ZEw2ZWpyNStmTEdLY3ZYWkJXcmF1dXQwXHJcbkV0WDQ2ODI1elorZHg1NXpPbm9Xa2cvSzBDQkpjM0prQ29ES0toVlVZQTZTRGtZbGF1Zz1cclxuLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLVxuIiwic2lnbmVkRmllbGRzIjoic2FsdCxkYXRhLGRpZ2VzdCx2YWxpZEZyb20sdmFsaWRUbyIsInZhbGlkRnJvbSI6IjIwMTgxMTEyMTQyOTI4IiwidmFsaWRUbyI6IjIwMTgxMTEyMTQzMTI4In0'
Copy link
Contributor

Choose a reason for hiding this comment

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

what does path do and what is this giant string?

src/App.js Outdated
<VncConsole host='localhost' port='3333' />
<VncConsole encrypt='true'
path='eyJzYWx0IjoicDNXeWRiNUswWm89IiwiZGF0YSI6IiU3QiUyMnBvcnQlMjIlM0ElMjI1OTAzJTIyJTJDJTIyaG9zdCUyMiUzQSUyMjEwLjM3LjE2MC41MSUyMiUyQyUyMnNzbF90YXJnZXQlMjIlM0FmYWxzZSU3RCIsInNpZ25hdHVyZSI6IjNaWHgzS0VpL1gyMEVpNXVHekpueENxbTRIcUlHV051Wk5KeUYwa29WbHBOUXRiWVFWeHUrY0VuSHNlM29YN1AwVzBlTWhZVTU5NlFDK01HNldHT0NPYUJURzU0TFNpOEtqL0t6dW9WODN5MVZVM0dUbU5CNDlCdE1pQ3VqYm5qaHNlcTdDdGJ2S0tsV3hZRGhPSkRQSEhHbkpSR3JGNzluSXNsU2JTM2hNc2xyUUJqQ0crMXN5bkI1UklWODJJSTA0TElvVjAxYTMwRi95S3hoZmFJRFhuTEc3aVVFNWV5T09RZWFPbjZXWjFRMlN6ZWtUbFpOZEhndlhpYkNNbjN6VXVGZ2RLWWpUNWhKZmdRa3VNNGFaQTk3Q3U2bDBRY0JZUitGbDZvYm5pTGJFMWNMbVR3ekR4Qm12K2s3L2hzNGNvc2Q1eUJDOHkxd3VpTzMwWlRFZz09IiwiZGlnZXN0Ijoic2hhMSIsImNlcnRpZmljYXRlIjoiLS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tXG5NSUlGQ0RDQ0EvQ2dBd0lCQWdJQ0VBRXdEUVlKS29aSWh2Y05BUUVMQlFBd1p6RUxNQWtHQTFVRUJoTUNWVk14SkRBaUJnTlZCQW9NXHJcbkczSm9aWFl1YkdGaUxtVnVaeTVpY25FdWNtVmthR0YwTG1OdmJURXlNREFHQTFVRUF3d3BZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxcclxuYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB1TkRjeE5qZ3dIaGNOTVRnd09UQTJNRGcxTmpVeVdoY05Nak13T0RFeU1EZzFOalV5V2pCaFxyXG5NUXN3Q1FZRFZRUUdFd0pWVXpFa01DSUdBMVVFQ2d3YmNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dE1Td3dLZ1lEXHJcblZRUUREQ05pY25FdFpHVjJMbkpvWlhZdWJHRmlMbVZ1Wnk1aWNuRXVjbVZrYUdGMExtTnZiVENDQVNJd0RRWUpLb1pJaHZjTkFRRUJcclxuQlFBRGdnRVBBRENDQVFvQ2dnRUJBT0ZFZFRUUGxmTzdUZGwwRlY4TGFWMHRTQjJqcUVmeVFmd0k1clEyTkR1VElERHVOOFh2R0xIZ1xyXG4rV2tWQnJ6aDFicTJMcW9QcUJUWkJaejhXZCtoU2FKU0MvRGZ6RWlXbVhtbTUxdWd4RFo3N0tVODV6dndoU2x5Z1hocnEzMERSSVdKXHJcbk4rdEpQUDRXSXJyUjBKQTFkbFZaSThHb1liT2hVcTEwNDBBVHJDU0tPQWdiTXBSR3F4SkhhejNnZ3FaUzliYyttajV2Y2tDbzh3eHhcclxudjllNUJpU1RZQ1dURUU4T3JvTHdndjZjZVp4YU9RQ2NPRjNSOXJhY0QvRlhmSDFYellvTWNWbUhnakNrZVlZdjBGblMxRHNWcjBOQlxyXG5sb0Z1eTBTa1FGajhxZnludjFmS0lIL0VNQTVYNGNOalpDNmNTVUpxWENFNjd4ZUMwU2JPK1pmTHkxRUNBd0VBQWFPQ0FjSXdnZ0crXHJcbk1CMEdBMVVkRGdRV0JCUTJzajc0d3RGaUdmTDVjZytmdTBEczIrcEg3ekNCbWdZSUt3WUJCUVVIQVFFRWdZMHdnWW93Z1ljR0NDc0dcclxuQVFVRkJ6QUNobnRvZEhSd09pOHZZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxibWN1WW5KeExuSmxaR2hoZEM1amIyMDZPREF2YjNacFxyXG5jblF0Wlc1bmFXNWxMM05sY25acFkyVnpMM0JyYVMxeVpYTnZkWEpqWlQ5eVpYTnZkWEpqWlQxallTMWpaWEowYVdacFkyRjBaU1ptXHJcbmIzSnRZWFE5V0RVd09TMVFSVTB0UTBFd2daSUdBMVVkSXdTQmlqQ0JoNEFVVmllZnFjK2djSFVBQ2dDRThsVVFRQUxTTldHaGE2UnBcclxuTUdjeEN6QUpCZ05WQkFZVEFsVlRNU1F3SWdZRFZRUUtEQnR5YUdWMkxteGhZaTVsYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB4TWpBd1xyXG5CZ05WQkFNTUtXSnljUzFrWlhZdWNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dExqUTNNVFk0Z2dJUUFEQUpCZ05WXHJcbkhSTUVBakFBTUE0R0ExVWREd0VCL3dRRUF3SUZvREFnQmdOVkhTVUJBZjhFRmpBVUJnZ3JCZ0VGQlFjREFRWUlLd1lCQlFVSEF3SXdcclxuTGdZRFZSMFJCQ2N3SllJalluSnhMV1JsZGk1eWFHVjJMbXhoWWk1bGJtY3VZbkp4TG5KbFpHaGhkQzVqYjIwd0RRWUpLb1pJaHZjTlxyXG5BUUVMQlFBRGdnRUJBRWI1V1NvU3BPd1JUZjdVNnlPLzdwNUIvT3RiZHQrdGtPei9WSFBhRm54YlcwTUMrR3hGdEI0dVBQNVNwVHNQXHJcblZkWDJ0aVRVeXlKU3RMbXRMVWxlTHYrM0ZVbW9NcDV2eis4UnU0REE4QzNzdlhrYkNTZS9Qd2JOOEFyYlN2eXI5aUMveUpmN25OQ2Rcclxuckh1bzEzWnpZNDEvdTlUSHhiSW5pNHZPT00xTGw0WDBwOVRSTko1bzBhSTl5djBoR2NTV21HT0QyUi9pNWk1SkJFMVFFaVVrVlJWUVxyXG5vS3l5QXVveExtNkQ4bkJKMWRrUzhHVUtuM3J4blFFUGNZd2I0dGdaS3V6bG03RkdMOWI3ZEw2ZWpyNStmTEdLY3ZYWkJXcmF1dXQwXHJcbkV0WDQ2ODI1elorZHg1NXpPbm9Xa2cvSzBDQkpjM0prQ29ES0toVlVZQTZTRGtZbGF1Zz1cclxuLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLVxuIiwic2lnbmVkRmllbGRzIjoic2FsdCxkYXRhLGRpZ2VzdCx2YWxpZEZyb20sdmFsaWRUbyIsInZhbGlkRnJvbSI6IjIwMTgxMTEyMTQyOTI4IiwidmFsaWRUbyI6IjIwMTgxMTEyMTQzMTI4In0'
host='brq-dev.rhev.lab.eng.brq.redhat.com' port='6100' />
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be variables

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@@ -78,6 +78,6 @@ export default connect(
(dispatch, { vm, consoleId }) => ({
onCheckConsoleSessionInUse: ({ usbFilter, userId }) => dispatch(checkConsoleInUse({ vmId: vm.get('id'), usbFilter, userId })),
onConsoleSessionConfirmaClose: () => dispatch(setConsoleInUse({ vmId: vm.get('id'), consoleInUse: null })),
onDownloadConsole: ({ usbFilter }) => dispatch(downloadConsole({ vmId: vm.get('id'), usbFilter, consoleId })),
onDownloadConsole: ({ usbFilter }) => dispatch(openConsole({ vmId: vm.get('id'), usbFilter: usbFilter, consoleId: consoleId })),
Copy link
Contributor

Choose a reason for hiding this comment

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

may want to rename onDownloadConsole to onOpenConsole

src/App.js Outdated
@@ -51,7 +51,9 @@ const App = ({ history, config, appReady }) => {
return (
<ConnectedRouter history={history}>
<div id='app-container'>
<VncConsole host='localhost' port='3333' />
<VncConsole encrypt='true'
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be a boolean not string, it means you can just write:

<VncConsole encrypt .... />

src/App.js Outdated
<VncConsole host='localhost' port='3333' />
<VncConsole encrypt='true'
path='eyJzYWx0IjoicDNXeWRiNUswWm89IiwiZGF0YSI6IiU3QiUyMnBvcnQlMjIlM0ElMjI1OTAzJTIyJTJDJTIyaG9zdCUyMiUzQSUyMjEwLjM3LjE2MC41MSUyMiUyQyUyMnNzbF90YXJnZXQlMjIlM0FmYWxzZSU3RCIsInNpZ25hdHVyZSI6IjNaWHgzS0VpL1gyMEVpNXVHekpueENxbTRIcUlHV051Wk5KeUYwa29WbHBOUXRiWVFWeHUrY0VuSHNlM29YN1AwVzBlTWhZVTU5NlFDK01HNldHT0NPYUJURzU0TFNpOEtqL0t6dW9WODN5MVZVM0dUbU5CNDlCdE1pQ3VqYm5qaHNlcTdDdGJ2S0tsV3hZRGhPSkRQSEhHbkpSR3JGNzluSXNsU2JTM2hNc2xyUUJqQ0crMXN5bkI1UklWODJJSTA0TElvVjAxYTMwRi95S3hoZmFJRFhuTEc3aVVFNWV5T09RZWFPbjZXWjFRMlN6ZWtUbFpOZEhndlhpYkNNbjN6VXVGZ2RLWWpUNWhKZmdRa3VNNGFaQTk3Q3U2bDBRY0JZUitGbDZvYm5pTGJFMWNMbVR3ekR4Qm12K2s3L2hzNGNvc2Q1eUJDOHkxd3VpTzMwWlRFZz09IiwiZGlnZXN0Ijoic2hhMSIsImNlcnRpZmljYXRlIjoiLS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tXG5NSUlGQ0RDQ0EvQ2dBd0lCQWdJQ0VBRXdEUVlKS29aSWh2Y05BUUVMQlFBd1p6RUxNQWtHQTFVRUJoTUNWVk14SkRBaUJnTlZCQW9NXHJcbkczSm9aWFl1YkdGaUxtVnVaeTVpY25FdWNtVmthR0YwTG1OdmJURXlNREFHQTFVRUF3d3BZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxcclxuYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB1TkRjeE5qZ3dIaGNOTVRnd09UQTJNRGcxTmpVeVdoY05Nak13T0RFeU1EZzFOalV5V2pCaFxyXG5NUXN3Q1FZRFZRUUdFd0pWVXpFa01DSUdBMVVFQ2d3YmNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dE1Td3dLZ1lEXHJcblZRUUREQ05pY25FdFpHVjJMbkpvWlhZdWJHRmlMbVZ1Wnk1aWNuRXVjbVZrYUdGMExtTnZiVENDQVNJd0RRWUpLb1pJaHZjTkFRRUJcclxuQlFBRGdnRVBBRENDQVFvQ2dnRUJBT0ZFZFRUUGxmTzdUZGwwRlY4TGFWMHRTQjJqcUVmeVFmd0k1clEyTkR1VElERHVOOFh2R0xIZ1xyXG4rV2tWQnJ6aDFicTJMcW9QcUJUWkJaejhXZCtoU2FKU0MvRGZ6RWlXbVhtbTUxdWd4RFo3N0tVODV6dndoU2x5Z1hocnEzMERSSVdKXHJcbk4rdEpQUDRXSXJyUjBKQTFkbFZaSThHb1liT2hVcTEwNDBBVHJDU0tPQWdiTXBSR3F4SkhhejNnZ3FaUzliYyttajV2Y2tDbzh3eHhcclxudjllNUJpU1RZQ1dURUU4T3JvTHdndjZjZVp4YU9RQ2NPRjNSOXJhY0QvRlhmSDFYellvTWNWbUhnakNrZVlZdjBGblMxRHNWcjBOQlxyXG5sb0Z1eTBTa1FGajhxZnludjFmS0lIL0VNQTVYNGNOalpDNmNTVUpxWENFNjd4ZUMwU2JPK1pmTHkxRUNBd0VBQWFPQ0FjSXdnZ0crXHJcbk1CMEdBMVVkRGdRV0JCUTJzajc0d3RGaUdmTDVjZytmdTBEczIrcEg3ekNCbWdZSUt3WUJCUVVIQVFFRWdZMHdnWW93Z1ljR0NDc0dcclxuQVFVRkJ6QUNobnRvZEhSd09pOHZZbkp4TFdSbGRpNXlhR1YyTG14aFlpNWxibWN1WW5KeExuSmxaR2hoZEM1amIyMDZPREF2YjNacFxyXG5jblF0Wlc1bmFXNWxMM05sY25acFkyVnpMM0JyYVMxeVpYTnZkWEpqWlQ5eVpYTnZkWEpqWlQxallTMWpaWEowYVdacFkyRjBaU1ptXHJcbmIzSnRZWFE5V0RVd09TMVFSVTB0UTBFd2daSUdBMVVkSXdTQmlqQ0JoNEFVVmllZnFjK2djSFVBQ2dDRThsVVFRQUxTTldHaGE2UnBcclxuTUdjeEN6QUpCZ05WQkFZVEFsVlRNU1F3SWdZRFZRUUtEQnR5YUdWMkxteGhZaTVsYm1jdVluSnhMbkpsWkdoaGRDNWpiMjB4TWpBd1xyXG5CZ05WQkFNTUtXSnljUzFrWlhZdWNtaGxkaTVzWVdJdVpXNW5MbUp5Y1M1eVpXUm9ZWFF1WTI5dExqUTNNVFk0Z2dJUUFEQUpCZ05WXHJcbkhSTUVBakFBTUE0R0ExVWREd0VCL3dRRUF3SUZvREFnQmdOVkhTVUJBZjhFRmpBVUJnZ3JCZ0VGQlFjREFRWUlLd1lCQlFVSEF3SXdcclxuTGdZRFZSMFJCQ2N3SllJalluSnhMV1JsZGk1eWFHVjJMbXhoWWk1bGJtY3VZbkp4TG5KbFpHaGhkQzVqYjIwd0RRWUpLb1pJaHZjTlxyXG5BUUVMQlFBRGdnRUJBRWI1V1NvU3BPd1JUZjdVNnlPLzdwNUIvT3RiZHQrdGtPei9WSFBhRm54YlcwTUMrR3hGdEI0dVBQNVNwVHNQXHJcblZkWDJ0aVRVeXlKU3RMbXRMVWxlTHYrM0ZVbW9NcDV2eis4UnU0REE4QzNzdlhrYkNTZS9Qd2JOOEFyYlN2eXI5aUMveUpmN25OQ2Rcclxuckh1bzEzWnpZNDEvdTlUSHhiSW5pNHZPT00xTGw0WDBwOVRSTko1bzBhSTl5djBoR2NTV21HT0QyUi9pNWk1SkJFMVFFaVVrVlJWUVxyXG5vS3l5QXVveExtNkQ4bkJKMWRrUzhHVUtuM3J4blFFUGNZd2I0dGdaS3V6bG03RkdMOWI3ZEw2ZWpyNStmTEdLY3ZYWkJXcmF1dXQwXHJcbkV0WDQ2ODI1elorZHg1NXpPbm9Xa2cvSzBDQkpjM0prQ29ES0toVlVZQTZTRGtZbGF1Zz1cclxuLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLVxuIiwic2lnbmVkRmllbGRzIjoic2FsdCxkYXRhLGRpZ2VzdCx2YWxpZEZyb20sdmFsaWRUbyIsInZhbGlkRnJvbSI6IjIwMTgxMTEyMTQyOTI4IiwidmFsaWRUbyI6IjIwMTgxMTEyMTQzMTI4In0'
host='brq-dev.rhev.lab.eng.brq.redhat.com' port='6100' />
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

{ type: 'INTRENAL_CONSOLE', payload: { vmId, consoleId } })
data.ticket = ticket
data.isSpice = isSpice
return data
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand why do you return here data?

}
}
let data = yield callExternalAction('consoleProxyTicket', Api.consoleProxyTicket,
{ type: 'INTRENAL_CONSOLE', payload: { vmId, consoleId } })
Copy link
Contributor

Choose a reason for hiding this comment

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

Use constants, please.

@bond95
Copy link
Contributor

bond95 commented Jan 9, 2019

@kozloffsky rebase, please

@gregsheremeta
Copy link
Contributor

@kozloffsky indeed, it's impossible to review unless it's rebased. I just released 1.5.0 (== master at this moment) so that's a nice clean place to start. Please schedule a video call with @bond95 for today if you need assistance.

@gregsheremeta
Copy link
Contributor

@bond95 @kozloffsky rebase AND squash

@kozloffsky kozloffsky force-pushed the RHV-1720 branch 2 times, most recently from a748b92 to c628987 Compare January 14, 2019 11:40
@@ -35,4 +35,5 @@ module.exports = {
appNodeModules: resolveApp('node_modules'),
ownNodeModules: resolveApp('node_modules'),
nodePaths: nodePaths,
novmc: resolveApp('node_modules/@novnc/novnc')
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace, please, 'm' with 'n'

@@ -100,7 +100,7 @@ module.exports = {
// Process JS with Babel.
{
test: /\.(js|jsx)$/,
include: paths.appSrc,
include: [paths.appSrc, paths.novmc],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@@ -96,7 +96,7 @@ module.exports = {
// Process JS with Babel.
{
test: /\.(js|jsx)$/,
include: paths.appSrc,
include: [paths.appSrc, paths.novmc],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

SET_CONSOLE_IN_USE,
SET_CONSOLE_LOGON,
} from '_/constants'
import { SET_CONSOLE_IN_USE, CHECK_CONSOLE_IN_USE, SET_CONSOLE_LOGON, SET_CONSOLE_TICKETS, DOWNLOAD_CONSOLE_VM } from '../constants'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to move each constant to separate line

@@ -150,9 +152,76 @@ const VmCreatePageConnected = connect(
})
)(VmCreatePage)

class VmConsoleSelector extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

This component doesn't belong here, please, move it to separate file.

})
},

consoleTicket ({ vmId, consoleId }: {vmId: string, consoleId: string}): Promise<Object> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after { and before }
{ vmId: string, consoleId: string }

@@ -12,6 +12,11 @@ const consoles = actionReducer(initialState, {
[SET_CONSOLE_LOGON] (state, { payload: { vmId, isLogon } }) {
return state.setIn(['vms', vmId, 'isLogon'], isLogon)
},
[SET_CONSOLE_TICKETS] (state, { payload: { vmId, proxyTicket, ticket } }) {
const tmpState = state.setIn(['vms', vmId, 'proxyTicket'], proxyTicket)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to do chain of setIn?

path: '/vm/:id/console/:console_id',
title: (match) => match.params.console_id,
component: VmConsolePage,
closeable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add type of the page.

let ticket = yield callExternalAction('consoleTicket', Api.consoleTicket,
{ type: 'INTRENAL_CONSOLE', payload: { vmId, consoleId } })
yield put(setConsoleTickets({ vmId, proxyTicket: data.proxy_ticket.value, ticket: ticket.ticket }))
logger.log(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this log is unnecessary, please, remove it

if (actionsCopy.length === 0) {
return null
}
return <SplitButton title='Spice Console' id={id}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Translate, please.

@gregsheremeta
Copy link
Contributor

@kozloffsky i see you rebased, but you didn't squash. It is not reviewable until you do. The commits are a mess and impossible to follow.

please squash before you do any more work on this. If you don't know what I'm talking about, please ping me or @bond95 .

type consoles, for others page begin config file download process.
Also added console selector on page toolbar.
@gregsheremeta
Copy link
Contributor

much better, thank you

@gregsheremeta
Copy link
Contributor

ci build please

@gregsheremeta
Copy link
Contributor

[needs ovirt-engine-nodejs-modules update for react-console]

@gregsheremeta
Copy link
Contributor

@kozloffsky I couldn't get the latest commits to run.

Production compile and installed rpm:
selection_332

Dev compile:
selection_327
selection_328
selection_329
selection_330
selection_331

@gregsheremeta gregsheremeta removed the request for review from mareklibra January 30, 2019 03:00
@gregsheremeta
Copy link
Contributor

closing, doesn't work

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.

Support web-based console connection
4 participants