You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by anshul1886 <gi...@git.apache.org> on 2015/08/10 09:38:58 UTC
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/669
Made the adding new keyboard language support easier
This branch has implemented following improvements in console proxy keyboard language support
1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it.
2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations.
3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #669
----
commit 94995312a97716417cd0bbe7c2a323aa5cc09f5e
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-04T04:54:16Z
Added support for copying js files from management server to console proxy VM with stop start
commit 9e5f1c7b071f48d3c608b314e8111f77a0018a0d
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-07T05:45:03Z
Added CopyFileInVmCommand support in ovm3 and simulator and added fallback path for js files
commit 130827f928cae4f56cd76a9431745f5959d50349
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-07T08:38:32Z
generalize console proxy keyboard support framework
and added the additional parameters which will be needed for keyboard mappings
commit 2d20c2c9b0bfa11157f3412080144df81d149438
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-10T06:27:03Z
Moved the console Keyboard Options to new file so that user can add keyboard options easily
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
tag:mergeready
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by anshul1886 <gi...@git.apache.org>.
GitHub user anshul1886 reopened a pull request:
https://github.com/apache/cloudstack/pull/669
Made the adding new keyboard language support easier
https://issues.apache.org/jira/browse/CLOUDSTACK-8665
This branch has implemented following improvements in console proxy keyboard language support
1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it.
2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations.
3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js.
FS for this feature is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy
After these changes how to add keyboard support for new language or fix existing broken keys WIP document is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Adding+support+for+non-US+Keyboard+for+Console+Proxy
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #669
----
commit 31e12325e715420536ee3215e312474be1b258a5
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-04T04:54:16Z
CLOUDSTACK-8665: Added following improvements:
1. Added support for copying js files from management server to console proxy VM with stop start
2. Generalise console keyboard support framework
3. Passing additional parameters which will be needed for keyboard mappings for vm console
4. Moved the console Keyboard Options to new file so that user can add keyboard options easily
5. Improved memory footprint, now keeping only required locale mappings
6. Added more conditions while setting up translation table
7. Improved browser detection for keyboard mappings
8. Formatted ajaxviewer.js and ajaxkeys.js with spaces instead of tabs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
@karuturi, We can merge this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
@DaanHoogland @rafaelweingartner Will have a look at the proposed changes and get back to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the issue:
https://github.com/apache/cloudstack/pull/669
@rafaelweingartner good improvement. @anshul1886 can you merge Rafael's stuff? He addresses the code doublure and the generic exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
@sureshanaparti, Can you have a look at this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612463
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
I got your point but I don't understand how will that make any difference here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-220025191
This shows that it does not break anything, but I don't think we have any verification that this feature works as expected yet...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-220024998
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 1
Errors: 0
Duration: 4h 18m 27s
```
**Summary of the problem(s):**
```
FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 842, in test_01_isolate_network_FW_PF_default_routes_egress_true
"Attempt to retrieve google.com index page should be successful!"
AssertionError: Attempt to retrieve google.com index page should be successful!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_A6H565/results.txt
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/DeployDataCenter__May_18_2016_08_23_35_7BOYAF/runinfo.txt)
**`/tmp/MarvinLogs/test_network_A6H565:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_network_A6H565/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_GFBFLL:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR669/tmp/MarvinLogs/test_vpc_routers_GFBFLL/runinfo.txt)
Uploads will be available until `2016-07-18 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36704745
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
--- End diff --
My note is related to catch block, once an io exception is caught, do we want to return false to callee ( inside the catch block)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611905
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
+ }
+
return true;
}
@Override
public boolean finalizeStart(VirtualMachineProfile profile, long hostId, Commands cmds, ReservationContext context) {
- CheckSshAnswer answer = (CheckSshAnswer)cmds.getAnswer("checkSsh");
- if (answer == null || !answer.getResult()) {
- if (answer != null) {
- s_logger.warn("Unable to ssh to the VM: " + answer.getDetails());
- } else {
- s_logger.warn("Unable to ssh to the VM: null answer");
+ for(Answer answer : cmds.getAnswers()) {
+ if(answer == null || !answer.getResult()) {
+ if (answer != null) {
+ s_logger.warn("Unable to ssh to the VM: " + answer.getDetails());
+ } else {
+ s_logger.warn("Unable to ssh to the VM: null answer");
+ }
+ if(cmds.getAnswer("copyFile") == answer) {
+ continue;
+ }
+ return false;
--- End diff --
Its returning false if the command is failed or has not returned answer and that is failure case. In case of copyFile we are ignoring the answer and continuing. So if block is making sense for that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r104846603
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
@rafaelweingartner No, that's not the case. Some hypervisors copy through hypervisor and some directly copies to system vm. Also can you give example how all this can be achieved through common code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r64009630
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
The message can be more specific and explanatory. The SshHelper shouldn't throw such a generic exception anyway. This is sloppy development and we now have a good chance to improve on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959351
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
can you catch more specific exceptions here, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611014
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
No. There is no file open operation involved here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710950
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
That line cannot through exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36832023
--- Diff: client/pom.xml ---
@@ -478,6 +478,12 @@
<copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/scripts">
<fileset dir="${basedir}/../scripts"/>
</copy>
+ <copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/systemvm/js">
--- End diff --
Do we have any unit-tests added for this change? As well, if we possible, can we have a document for the console proxy workflow available on wiki ? This may help people to know the exact workflow or class interactions with business flow.
As well, a small change to this request message "Made the adding new keyboard language support easier" could be beneficial i believe, helps people to know the complete context for this change.
Note :this is not specific to this line, but a generic note.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-216186163
@anshul1886 please rebase against latest master and share status of your PR
This looks like an interesting PR we should have
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609278
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
@sedukull Could you please elaborate more on what is repetitive here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609071
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
--- End diff --
As well, here if its a directory, we are using multiple file objects, are these auto closed and resources consumed are released back or does the api for File..provides that facility?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36608900
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
This code seems repetitive, can this be made as a lib, or convert to a factory based upon given hypervisor type and use that pattern in each command execute call?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611238
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
+ }
+
return true;
}
@Override
public boolean finalizeStart(VirtualMachineProfile profile, long hostId, Commands cmds, ReservationContext context) {
- CheckSshAnswer answer = (CheckSshAnswer)cmds.getAnswer("checkSsh");
- if (answer == null || !answer.getResult()) {
- if (answer != null) {
- s_logger.warn("Unable to ssh to the VM: " + answer.getDetails());
- } else {
- s_logger.warn("Unable to ssh to the VM: null answer");
+ for(Answer answer : cmds.getAnswers()) {
+ if(answer == null || !answer.getResult()) {
+ if (answer != null) {
+ s_logger.warn("Unable to ssh to the VM: " + answer.getDetails());
+ } else {
+ s_logger.warn("Unable to ssh to the VM: null answer");
+ }
+ if(cmds.getAnswer("copyFile") == answer) {
+ continue;
+ }
+ return false;
--- End diff --
It seems this return seems to be inside of if..block, so for loop effectively has no effect and returns for one run?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/669
@anshul1886 you asked to show you how it can be done.
I opened a PR https://github.com/anshul1886/cloudstack-1/pull/1 for the branch you are using in this PR. Can you take a look?
OVM (not implemented) and XenServer has a different flow of execution (I would like to check that later)
So, what do you think of a code like the one I showed you?
@DaanHoogland could you also take a look at the proposal?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611175
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
--- End diff --
should we return false in case of an exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705867
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
My note was related to the below example.
example_dir_for_iteration contains say 5 entries as follows (dira, text_file_a,socket_file_a,block_device_file_a,zip_file_a), in else block of the code, we are copying all files. Now, in your case where you explicitly mentioned key file path(/root/..) these file types may not be possible, but these type of copying without file types checks, leads to vulnerabilities in the application long run. So, i mentioned to see we are copying files as mentioned in else block, do we want to add some file type check that we are interesting in copying, for that i gave an example where directory contains multiple file types.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612869
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
Here if it is a file then I am copying it directly as can be seen in else block but if it is a directory then I am listing files and then copying files individually as the library which we are using elsewhere in CloudStack doesn't support copying directories.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36839282
--- Diff: client/pom.xml ---
@@ -478,6 +478,12 @@
<copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/scripts">
<fileset dir="${basedir}/../scripts"/>
</copy>
+ <copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/systemvm/js">
--- End diff --
Thanks for the FS link, it was helpful. Regarding the unit test, i would disagree to say that they are meant to test corner cases. Not sure, what it doesnt make sense to you. In general, unit test purpose is not what you meant, in this commit, if we see we modified near to 28 code files, added new classes and code, it makes sense to have unit tests for the added code. It will help to test the code units and individual functional blocks during build, these areas not always are touched by functional tests. It adds to the coverage, make sure code modified down the lane adheres to the contract through failures in tests etc. Its appreciated, if we can have few unit tests added to this, right now they are zero tests for this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36949815
--- Diff: client/pom.xml ---
@@ -478,6 +478,12 @@
<copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/scripts">
<fileset dir="${basedir}/../scripts"/>
</copy>
+ <copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/systemvm/js">
--- End diff --
My understanding about unit testing is similar to mentioned in this blog http://simpleprogrammer.com/2010/10/15/the-purpose-of-unit-testing/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-220447959
I think with @milamberspace's review, we are only missing one code review to get this one in. Thanks everyone...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by milamberspace <gi...@git.apache.org>.
Github user milamberspace commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-220446031
LGTM.
Tested manually with a real test deployment of CS49, with my French azerty keyboard: works fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63961096
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ExecutionResult;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class CitrixCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, CitrixResourceBase> {
+
+ private static final Logger s_logger = Logger.getLogger(CitrixCopyFileInVmCommandWrapper.class);
+
+ @Override
+ public Answer execute(final CopyFileInVmCommand cmd, final CitrixResourceBase citrixResourceBase) {
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for(File f : FileUtils.listFiles(file, TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ ExecutionResult result = citrixResourceBase.copyFileInVm(cmd.getVmIp(), f, cmd.getDest());
+ if(!result.isSuccess()) {
+ return new CopyFileInVmAnswer(cmd, result.getDetails());
+ }
+ }
+ } else {
+ ExecutionResult result = citrixResourceBase.copyFileInVm(cmd.getVmIp(), file, cmd.getDest());
+ if(!result.isSuccess()) {
+ return new CopyFileInVmAnswer(cmd, result.getDetails());
+ }
+ }
+ }
+
+ } catch (Exception e) {
--- End diff --
please catch more specific exceptions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by milamberspace <gi...@git.apache.org>.
Github user milamberspace commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63469835
--- Diff: ui/consoleKeyboardOptions.jsp ---
@@ -0,0 +1,24 @@
+<%--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ --%>
+<option value=""></option>
+<option value="us"><fmt:message key="label.standard.us.keyboard" /></option>
+<option value="uk"><fmt:message key="label.uk.keyboard" /></option>
+<option value="jp"><fmt:message key="label.japanese.keyboard" /></option>
+<option value="sc"><fmt:message key="label.simplified.chinese.keyboard" /></option>
+<option value="fr">French AZERTY keyboard</option>
--- End diff --
That will be better if the French label use a localization key from messages.properties like the other languages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988285
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1298,6 +1304,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
Answered above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
@jburwell Failed tests are not related to change in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705320
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
I mean the below code is used across few code files in same form or some modified form, thats where i made a note, to avoid duplication. If its possible to make one common definition and use across.
File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for(File f : FileUtils.listFiles(file, TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ ExecutionResult result = citrixResourceBase.copyFileInVm(cmd.getVmIp(), f, cmd.getDest());
+ if(!result.isSuccess()) {
+ return new CopyFileInVmAnswer(cmd, result.getDetails());
+ }
+ }
+ } else {
+ ExecutionResult result = citrixResourceBase.copyFileInVm(cmd.getVmIp(), file, cmd.getDest());
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by anshul1886 <gi...@git.apache.org>.
GitHub user anshul1886 reopened a pull request:
https://github.com/apache/cloudstack/pull/669
Made the adding new keyboard language support easier
https://issues.apache.org/jira/browse/CLOUDSTACK-8665
This branch has implemented following improvements in console proxy keyboard language support
1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it.
2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations.
3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js.
FS for this feature is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy
After these changes how to add keyboard support for new language or fix existing broken keys WIP document is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Adding+support+for+non-US+Keyboard+for+Console+Proxy
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #669
----
commit 31e12325e715420536ee3215e312474be1b258a5
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-04T04:54:16Z
CLOUDSTACK-8665: Added following improvements:
1. Added support for copying js files from management server to console proxy VM with stop start
2. Generalise console keyboard support framework
3. Passing additional parameters which will be needed for keyboard mappings for vm console
4. Moved the console Keyboard Options to new file so that user can add keyboard options easily
5. Improved memory footprint, now keeping only required locale mappings
6. Added more conditions while setting up translation table
7. Improved browser detection for keyboard mappings
8. Formatted ajaxviewer.js and ajaxkeys.js with spaces instead of tabs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r104768693
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +602,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
--- End diff --
What about instantiating the response right away `CopyFileInVmAnswer copyFileInVmAnswer = new CopyFileInVmAnswer(cmd)` in the first lines? Then, you could reduce this cyclomatic complexity here (if/if/for). You could negate the Ifs, if the negation is true, you then return `copyFileInVmAnswer `.
what do you think of that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36612331
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
--- End diff --
We are copying some files (using pom.xml) in that folder so these files are expected there. If these files do not exist there then he may have forgotten to add there those file. Returning false will make him identify the issue because of that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705033
--- Diff: systemvm/js/ajaxviewer.js ---
@@ -184,7 +205,7 @@ KeyboardMapper.prototype = {
if(eventType == AjaxViewer.KEY_UP && (code == AjaxViewer.JS_KEY_ALT || code == AjaxViewer.JS_KEY_CTRL))
this.mappedInput.push({type : eventType, code: this.jsX11KeysymMap[code], modifiers: modifiers});
- } else if(eventType == AjaxViewer.KEY_PRESS && guestos == 'null') {
+ } else if(eventType == AjaxViewer.KEY_PRESS && guestos.toLowerCase() != 'windows') {
--- End diff --
This might be existing code, but for more cleaner way, do we want to remote all os strings and make them as constants or sort of enums defined at one place and use them with reference here?
something like:
gustos.toLowerCase() != OsTypes.WINDOWS
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611046
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
--- End diff --
same as above answer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63960622
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1298,6 +1304,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
please catch more specific exception(s)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959496
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
please, catch more specific exceptions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63960701
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -5402,4 +5402,20 @@ public boolean attachConfigDriveToMigratedVm(Connection conn, String vmName, Str
}
-}
+ public ExecutionResult copyFileInVm(String vmIp, File file, String dest) {
+ final Connection conn = getConnection();
+ final String hostPath = "/tmp/";
+
+ s_logger.debug("Copying file to VM with ip " + vmIp + " using host " + _host.getIp());
+ try {
+ SshHelper.scpTo(_host.getIp(), 22, _username, null, _password.peek(), hostPath, file.getCanonicalPath(), null);
+ } catch (final Exception e) {
--- End diff --
please catch more specific exceptions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-221861017
Can I get one more review on this one? I think we are basically set otherwise. Thx...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-131683677
@sedukull , Can you give a LGTM now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710619
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
This is hypervisor specific code and each hypervisor is achieving it differently. To me it doesn't make sense to put it in lib or something like that as it will unnecessarily add complexity without achieving much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/669
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
Added the missing license on one js file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/669
@anshul1886 it appears that there was a push to get this PR into 4.9, but @bvbharat found5 test failures and 3 skips. What is the status of resolving these issues?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-221763988
This one needs some work. We have merge conflicts and we need another code review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-219793270
@anshul1886 can you review the Jenkin errors and close and reopen the PR if they do not seem relevant to kick off the Jenkins run again. Thx...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-218869670
This PR currently have merge conflicts. Can you rebase please? Thx...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36711314
--- Diff: systemvm/js/ajaxviewer.js ---
@@ -184,7 +205,7 @@ KeyboardMapper.prototype = {
if(eventType == AjaxViewer.KEY_UP && (code == AjaxViewer.JS_KEY_ALT || code == AjaxViewer.JS_KEY_CTRL))
this.mappedInput.push({type : eventType, code: this.jsX11KeysymMap[code], modifiers: modifiers});
- } else if(eventType == AjaxViewer.KEY_PRESS && guestos == 'null') {
+ } else if(eventType == AjaxViewer.KEY_PRESS && guestos.toLowerCase() != 'windows') {
--- End diff --
Plan is to remove all OS strings and don't use them specifically at all.
I have left it here like this as removing this may break existing key mappings. Getting key mappings right is time consuming and this code was written to get them right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63997069
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
I understand you are basing yourself on bad code. That does not make it a good practice. Either the SshHelper should be refactored to some extend and the possible IOException should be caught seperately. See my general comment to this PR, please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r104770076
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ExecutionResult;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class CitrixCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, CitrixResourceBase> {
+
+ private static final Logger s_logger = Logger.getLogger(CitrixCopyFileInVmCommandWrapper.class);
+
+ @Override
+ public Answer execute(final CopyFileInVmCommand cmd, final CitrixResourceBase citrixResourceBase) {
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
duplicated lines here as well
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/669
### ACS CI BVT Run
**Sumarry:**
Build Number 382
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=0
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_non_contigiousvlan.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_routers_network_ops.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
### ACS CI BVT Run
**Sumarry:**
Build Number 134
Hypervisor xenserver
NetworkType Advanced
Passed=68
Failed=5
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_vpc_vpn.py
* ContextSuite context=TestRVPCSite2SiteVpn>:setup Failing since 10 runs
* ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 10 runs
* ContextSuite context=TestVpcSite2SiteVpn>:setup Failing since 10 runs
* test_volumes.py
* test_06_download_detached_volume Failed
* test_vm_life_cycle.py
* test_10_attachAndDetach_iso Failed
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988259
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
Answered above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609308
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
Should this be moved inside to end of try block?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63988200
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
SshHelper is throwing Exception so I am catching Exception. If you are talking about handling it here and not passing to CopyFileInVmAnswer then CopyFileInVmAnswer is just passing the exception message and not full exception. Applicable constructor from CopyFileInVmAnswer
public CopyFileInVmAnswer(CopyFileInVmCommand cmd, Exception e) {
super(cmd, false, e.getMessage());
}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/669
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36711116
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
--- End diff --
I think this case is also similar to else case and should be considered like that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-219672865
@milamberspace Added the localisation key in message.properties and now using it in consoleKeyboardOptions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-220464828
Really like the functionality so I think we must get this in. I have some remarks about the code that I stand by even though they are not entirely to blame on @anshul1886 as he calls methods fthrowing Exception, rom SshHelper mainly. It is a bad smell due to the older code and will not facilitate trouble shooting for the end user. So I would like to see the exception-handlers be enriched with some better diagnosis and *maybe* messages. nothing else to nag about.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63999962
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -594,6 +597,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
--- End diff --
I don't understand one point what difference it would make to catch specific actions and take same action in all case then catching generic exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-219918785
@swill They are failing on a test which don't even exist in source code. That test is in PR #1360 which is not yet merged so looks like some cleanup issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-218980649
@swill Its showing lot of conflicts currently and will take some time to resolve those. Will update PR once I will get some time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-175664522
@sedukull @anshul1886 please rebase against latest branch and help re-review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36833745
--- Diff: client/pom.xml ---
@@ -478,6 +478,12 @@
<copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/scripts">
<fileset dir="${basedir}/../scripts"/>
</copy>
+ <copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/systemvm/js">
--- End diff --
FS https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy has details regarding console proxy workflow which is present in description.
In developer setup these files are always present so adding unit test for certain (which will always pass) thing doesn't make sense to me. In my opinion unit test are meant to test corner cases or some uncertainty.
I will try to improve description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-221780994
@swill Resolved the merge conflicts. They were because of import statements.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by kiwiflyer <gi...@git.apache.org>.
Github user kiwiflyer commented on the issue:
https://github.com/apache/cloudstack/pull/669
tag:needslove
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-219129109
Thanks @anshul1886. Keep in mind that the freeze date for 4.9 is Monday next week.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36952117
--- Diff: client/pom.xml ---
@@ -478,6 +478,12 @@
<copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/scripts">
<fileset dir="${basedir}/../scripts"/>
</copy>
+ <copy todir="${basedir}/target/generated-webapp/WEB-INF/classes/systemvm/js">
--- End diff --
Its good that it does not say that UT are meant to test "corner" cases or "some uncertainity", as per your earlier understanding. It talks about TDD, in which UT are not meant to be written post the code, its good to follow TDD if we can write tests before the code. But if we missed it, and If we feel there is a value add in writing some tests, which can benefit others and helps testing the code in long run, its appreciated to add few, there is no harm. We could see many commits coming are having UT recently. Some even made sure to get near 100% coverage for their code. We know its not a new distinct module, its up to you and whatever the community decides!!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710980
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -1425,19 +1423,39 @@ public boolean finalizeCommandsOnStart(Commands cmds, VirtualMachineProfile prof
CheckSshCommand check = new CheckSshCommand(profile.getInstanceName(), controlNic.getIp4Address(), 3922);
cmds.addCommand("checkSsh", check);
+ try {
+ File uiFiles = new File("systemvm/js");
+ if (!uiFiles.exists()) {
+ uiFiles = new File("/usr/share/cloudstack-common/systemvm/js");
+ }
+ if (uiFiles.exists() && uiFiles.isDirectory()) {
+ CopyFileInVmCommand copyFile = new CopyFileInVmCommand(uiFiles.getCanonicalPath(), "/usr/local/cloud/systemvm/js", controlNic.getIp4Address());
+ cmds.addCommand("copyFile", copyFile);
+ } else {
+ s_logger.error("Couldn't locate localization files for console proxy");
+ return false;
+ }
+ } catch (IOException e) {
+ s_logger.error("Failed to copy localization files for console proxy: " + e.getMessage());
--- End diff --
I think above answer is still valid for your concern.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36710924
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
This is written mainly for copying js files to CPVM. And those js needs to be present on cloudstack installation. If somebody is able to manipulate CloudStack installation machine then that is major issue and we can't do anything about that. This makes me to believe that it doesn't makes much sense to do some file validation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609212
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
If its not a directory, but can be a block or socket file possible? Is there any criteria check to be added for file type to be copied?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
tag:mergeready
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r63959786
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
please replace the two lines above with
```try (File file = new File(cmd.getSrc());) {```
to make sure the file resource is closed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r104769820
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
@anshul1886 please reduce these duplicated lines.
Lines 42-57 here are the same as lines 608 - 622 on the class HypervDirectConnectResource.java.
I know you already said these are objects dealing with different hypervisors. However, the point here is that the process is the same no matter the hypervisor; thus, it is interesting to avoid such redundancy of code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/669
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36705202
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
Its not a big issue as such, but iam seeing like if there is an exception at liine 617: "return new CopyFileIn.. (cmd)", then catch will still throw your new object created. Again, its not a great issue to be worry about!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611512
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
I mean in case a copyfilein..if moved inside try block throws an exception , then the code return new...copyfile inside catch still works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611296
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
If its just a file then else part will take care. And I don't understand how does socket file is involved here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611140
--- Diff: plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java ---
@@ -595,6 +597,25 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
}
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ File keyFile = getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null);
+ }
+ }
+ } catch (Exception e) {
+ s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e);
+ return new CopyFileInVmAnswer(cmd, e);
+ }
+ return new CopyFileInVmAnswer(cmd);
--- End diff --
What purpose will it serve?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36611661
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCopyFileInVmCommandWrapper.java ---
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CopyFileInVmAnswer;
+import com.cloud.agent.api.CopyFileInVmCommand;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.ssh.SshHelper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.filefilter.TrueFileFilter;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+@ResourceWrapper(handles = CopyFileInVmCommand.class)
+public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> {
+
+ private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class);
+
+ @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) {
+ final File keyFile = new File("/root/.ssh/id_rsa.cloud");
+ try {
+ File file = new File(cmd.getSrc());
+ if(file.exists()) {
+ if(file.isDirectory()) {
+ for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) {
+ SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null);
+ }
+ } else {
--- End diff --
I mean we are checking whether its directory or not, but if its not a directory, other types can come EX: block, socket, or other file types defined by attributes ( like d,b etc), i mean do we have a specific requirement to copy with specific file type. EX: if we have group of files ( directory, block type, socket type , plain text files etc) , with above logic it returns all which is not of directory "type", iam inquiring to see we want to be explicit in code for a given file type?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by sedukull <gi...@git.apache.org>.
Github user sedukull commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r36609019
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
Does file object consumes a resource to be flushed or closed post the usage or its auto closed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #669: Made the adding new keyboard language support ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/669#discussion_r104769967
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1279,6 +1285,27 @@ private static DiskTO getIsoDiskTO(DiskTO[] disks) {
return null;
}
+ private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) {
+ VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
+ File keyFile = mgr.getSystemVMKeyFile();
+ try {
+ File file = new File(cmd.getSrc());
--- End diff --
Same duplicated lines here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/669
@swill @jburwell Updated in according to newer changes in UI. Also moved consoleKeyboardOptions to js file after jsp change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the issue:
https://github.com/apache/cloudstack/pull/669
Tested manually. LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
GitHub user anshul1886 reopened a pull request:
https://github.com/apache/cloudstack/pull/669
Made the adding new keyboard language support easier
https://issues.apache.org/jira/browse/CLOUDSTACK-8665
This branch has implemented following improvements in console proxy keyboard language support
1) ajaxviewer.js and ajaxkeys.js are main files involved in key code translations. These files now can be copied in systemvm/js folder and they will be copied to CPVM with stop/start performed on it.
2) Started passing parameters to CPVM needed to resolve the ambiguous cases of keycode translations.
3) Generalise the framework such that one needs to modify only ajaxkeys.js (file which has keycode mappings) without need of much knowledge in js.
FS for this feature is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy
After these changes how to add keyboard support for new language or fix existing broken keys WIP document is available at https://cwiki.apache.org/confluence/display/CLOUDSTACK/Adding+support+for+non-US+Keyboard+for+Console+Proxy
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 nonuskeyboardsupport
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #669
----
commit 715f18a1f26b7cccea8a8221a6cafee14e91f3a3
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-04T04:54:16Z
CLOUDSTACK-8665: Added following improvements:
1. Added support for copying js files from management server to console proxy VM with stop start
2. Generalise console keyboard support framework
3. Passing additional parameters which will be needed for keyboard mappings for vm console
4. Moved the console Keyboard Options to new file so that user can add keyboard options easily
5. Improved memory footprint, now keeping only required locale mappings
6. Added more conditions while setting up translation table
7. Improved browser detection for keyboard mappings
8. Formatted ajaxviewer.js and ajaxkeys.js with spaces instead of tabs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Made the adding new keyboard language sup...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/669#issuecomment-219620496
@swill Rebased against the current master and also squashed the commits into one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #669: Made the adding new keyboard language support easier
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the issue:
https://github.com/apache/cloudstack/pull/669
@anshul1886 Please address the open questions and rebase. Post the latest test results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---