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 12:47:37 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8721: Fixed Setting details of...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/673
CLOUDSTACK-8721: Fixed Setting details of VM through API results in removal of all other details except the one passed in API
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-8721
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/673.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 #673
----
commit 728635a3d3c4a4080ebcbcfbee54c0450b10012d
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-08-10T10:43:42Z
CLOUDSTACK-8721: Fixed Setting details of VM through API results in removal of all other details except the one passed in API
----
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134925254
@bhaisaab What else is needed 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134929360
@anshul1886 can you read and reply to some of the comments?
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134936210
@anshul1886 the only concern here is that this sort of breaks the API behaviour as in the past the details would set details (removing old details).
LGTM; we can merge once anyone else can review/test 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134931329
@bhaisaab Lost in discussion and got only end result that all we need is 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-129868539
Thanks @anshul1886 will test it soon.
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37181982
--- Diff: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java ---
@@ -136,13 +136,13 @@ public String getInstanceName() {
return instanceName;
}
- public Map getDetails() {
+ public Map<String, String> getDetails() {
--- End diff --
how about taking the opportunity to change this to
public Map<String, String> getDetails() {
if (this.details == null || this.details.isEmpty()) {
return null;
}
return details;
}
Is there any reason to pass through the crazy construct of getting the pointer to the first element and casting it back to a map? It seems like very unsafe. It becomes even more unsafe when using an instance of a generic 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/673
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-129572848
@anshul1886 I want to test this but haven't used the 'details' feature before. In what format am I supposed to send the key/value pairs? If you have a CloudMonkey or Marvin example, that would be great. I'll then verify it in my lab. Thanks!
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-129925883
@anshul1886 Tested your code, works just fine! LGTM.
The only thing I noticed: excisting key/values are removed, then added again. So the net result is OK. It just inserts them again (the auto-increment goes up) but that wasn't introduced by your code as far as I can see.
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37964142
--- Diff: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java ---
@@ -136,13 +136,13 @@ public String getInstanceName() {
return instanceName;
}
- public Map getDetails() {
+ public Map<String, String> getDetails() {
--- End diff --
This is to avoid long nested type casting before using details.
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37964980
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -2188,7 +2188,13 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
}
if (details != null && !details.isEmpty()) {
- vmInstance.setDetails(details);
+ _vmDao.loadDetails(vmInstance);
--- End diff --
When do we need to remove detail? Even if needed then those values can be updated to get the desired effect. Many details are for internal purposes only and user is not even aware of them.
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37183568
--- Diff: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java ---
@@ -136,13 +136,13 @@ public String getInstanceName() {
return instanceName;
}
- public Map getDetails() {
+ public Map<String, String> getDetails() {
--- End diff --
The CommandType.MAP API args are usually map of object returning objects; in practice AFAIR all map args are string->string. Either way, I don't understand this change in getDetails(), this at least will result in returning a new Map, so likely the author wants their details to be sort of read-only? If so, why not use final?
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37182838
--- Diff: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java ---
@@ -136,13 +136,13 @@ public String getInstanceName() {
return instanceName;
}
- public Map getDetails() {
+ public Map<String, String> getDetails() {
--- End diff --
@bhaisaab can you comment on 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134919656
Ping, update on 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134940325
ah, I see merging 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37183610
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -2188,7 +2188,13 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
}
if (details != null && !details.isEmpty()) {
- vmInstance.setDetails(details);
+ _vmDao.loadDetails(vmInstance);
--- End diff --
I think all we need is here, the side effect is that now users won't be able to remove any details; only add/update details.
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/673#discussion_r37183139
--- Diff: api/src/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java ---
@@ -136,13 +136,13 @@ public String getInstanceName() {
return instanceName;
}
- public Map getDetails() {
+ public Map<String, String> getDetails() {
--- End diff --
come to think of it
```
public Map<String, String> getDetails() {
return details;
}
```
should even do
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-129696884
@remibergsma Using cloudmonkey we can pass as shown below
update virtualmachine id=00f19661-da81-40b2-a07c-e46967421195 details[0].keyboard=fr
Here detail which we want to update has key "keyboard" and its value is "fr"
And using API
http://localhost:8096/api/?command=updateVirtualMachine&id=00f19661-da81-40b2-a07c-e46967421195&details[0].keyboard="fr"
---
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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-134938776
@remibergsma @bhaisaab Remi has already tested this and has given the 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: CLOUDSTACK-8721: Fixed Setting details of...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/673#issuecomment-129407249
cloudstack-pull-requests #945 failure is because of some random error in git fetching
---
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.
---