Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1518
---
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
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220652018
@swill Travis passed. Looks like it is ready to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220619741
For some reason Travis is unhappy. Would you mind trying again. Sorry for
the inconvenience...
---
If your project is set up for it, you can reply to this email
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220615109
@swill nothing outstanding 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
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r64042208
--- Diff:
engine/storage/src/org/apache/cloudstack/storage/image/NfsImageStoreDriverImpl.java
---
@@ -0,0 +1,30 @@
+package
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220608044
Thanks @swill @koushik-das! Sorry I was missing a license header, I pushed
it now.
---
If your project is set up for it, you can reply to this email and have
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220595138
This is a passing CI now. @koushik-das do you have anything outstanding
with this PR?
@nvazquez can you close and reopen or do a force push to kick off a
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220594320
### CI RESULTS
```
Tests Run: 83
Skipped: 0
Failed: 0
Errors: 2
Duration: 8h 53m 51s
```
**Summary of the
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220562852
@nvazquez Thanks for the fix. As @DaanHoogland mentioned please add license
header to the newly added file.
---
If your project is set up for it, you can
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63955721
--- Diff:
engine/storage/src/org/apache/cloudstack/storage/image/NfsImageStoreDriverImpl.java
---
@@ -0,0 +1,30 @@
+package
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-220427628
@koushik-das I pushed new changes including an intermediate class as you
suggested
@swill hopefully CI passes now
---
If your project is set up for it, you
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219754000
I am having a hard time testing this because I can't seem to get the
templates to become ready using this PR. My CI ran all night last night and
this morning it was
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219692208
@nvazquez I am not too familiar with the VMware specific code, rest of the
code looks ok.
Also a comment on #1361 when this feature was initially
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63494164
--- Diff:
engine/storage/src/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java
---
@@ -79,6 +80,25 @@
VMTemplateZoneDao
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219466200
@GabrielBrascher thanks for your review! I pushed changes fixing issues
you've found.
@koushik-das I've replied your comment, sorry for late reply
---
If
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63377708
--- Diff:
engine/storage/src/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java
---
@@ -79,6 +80,25 @@
VMTemplateZoneDao
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219249718
@nvazquez sorry for the late review,
I could point just 2 typos on Javadoc and some variables with the
underscore (would be nice to change those
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63281291
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageSubsystemCommandHandler.java
---
@@ -61,11 +61,28 @@ public
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63281246
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -303,6 +305,7 @@
protected
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r63281205
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -519,6 +523,64 @@ public Answer
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219154273
This doesn't seem to be related to this PR.Can you post
managment-server.log extract around the failure time?
---
If your project is set up for it, you can reply
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219137064
I have tried to CI this PR a couple times. I get the following during
Deploy DC.
```
Deploy DC Started
Exception Occurred :['Traceback (most
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219093988
I will run this through my KVM CI right 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
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-219083470
@koushik-das I only use Vmware hypervisor so I couldn't test it for Xen or
KVM. However, I reviewed code and found
`copySnapshotToTemplateFromNfsToNfsXenserver`
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-218129820
Looks like nfs version is only supported for VMware. If XS or KVM clusters
are added to the zone (having nfs version set on image store) will there be any
side
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r62652337
--- Diff:
services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
---
@@ -984,7 +984,8 @@ public
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r62651265
--- Diff:
engine/storage/src/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java
---
@@ -79,6 +80,25 @@
VMTemplateZoneDao
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r62649582
--- Diff:
engine/storage/image/src/org/apache/cloudstack/storage/image/TemplateServiceImpl.java
---
@@ -638,7 +638,9 @@ protected Void
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-218052341
@koushik-das Will you be able to review this PR? It has been waiting for
2d LGTM for a while now.
---
If your project is set up for it, you can reply to this
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-216650029
All checks passed. @rhtyd, @wido or @GabrielBrascher can you review and
give second OK.
---
If your project is set up for it, you can reply to this email and
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-216353485
@rafaelweingartner I squashed them, thanks for your comments and reviews!
---
If your project is set up for it, you can reply to this email and have your
reply
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-216351223
Just an update,
I forgot to ask, can you squash the commits?
I think this one it is better to have all of the changes into a single
commit.
---
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-216347918
@nvazquez, that is great.
As always your PRs are perfect, great job;)
LGTM from me here.
---
If your project is set up for it, you can reply to
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61790055
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +153,60 @@ public void
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61788817
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +153,60 @@ public void
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61788211
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +153,60 @@ public void
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-216293439
@rafaelweingartner @cristofolini I refactored unit tests based on your
comments, I also removed testStorageNfsVersionNotPresent as it is
covered by tests
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61756168
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +154,79 @@ public void
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61756291
--- Diff:
core/src/com/cloud/agent/api/storage/StorageNfsVersionCommand.java ---
@@ -0,0 +1,44 @@
+//
+// Licensed to the Apache Software
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61756086
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +154,79 @@ public void
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61754369
--- Diff:
core/src/com/cloud/agent/api/storage/StorageNfsVersionCommand.java ---
@@ -0,0 +1,44 @@
+//
+// Licensed to the Apache
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61754039
--- Diff:
core/src/com/cloud/agent/api/storage/StorageNfsVersionCommand.java ---
@@ -0,0 +1,44 @@
+//
+// Licensed to the Apache Software
Github user cristofolini commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61694862
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +154,79 @@ public void
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61646120
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +154,79 @@ public void
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61645464
--- Diff:
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java
---
@@ -117,4 +154,79 @@ public void
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1518#discussion_r61643519
--- Diff:
core/src/com/cloud/agent/api/storage/StorageNfsVersionCommand.java ---
@@ -0,0 +1,44 @@
+//
+// Licensed to the Apache
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-215852047
@rafaelweingartner I added test cases and the new hierarchy
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-215116783
@nvazquez I believe so.
This way we can reduce a little bit of code.
---
If your project is set up for it, you can reply to this email and have your
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-215116050
Thanks @rafaelweingartner, I like the idea, I think the best way would be
leave hierarchy like this:
- Command
-- NEW CLASS
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214868283
@nvazquez , what about extracting that âprivate Integer nfsVersionâ to
a common superclass to all of those command class? I have created a hierarchy
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214864175
Thanks @wido!
---
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
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214853978
Thanks! I'm not familiar with the VMWare code in CloudStack, so I won't
give a LGTM. But passing a version as a Int seems a lot better.
---
If your project is set up
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214806891
Cool, I changed it and pushed again
---
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
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214781618
@nvazquez If you know it's a number, pass it as an int. That way you will
never have a garbage string ending up somewhere. We have types for a reason :)
---
If your
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214779424
You're right, I just wanted to avoid parsing but I can change it to an
Integer
---
If your project is set up for it, you can reply to this email and have your
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1518#issuecomment-214734679
A version is a number, eg 3 or 4. Shouldn't the version be passed as a
integer through the code instead of a String?
---
If your project is set up for it, you can
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1518
CLOUDSTACK-9368: Fix for Support configurable NFS version for Secondary
Storage mounts
## Description
JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9368
This pull
57 matches
Mail list logo
