[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rafaelweingartner]

Github user rafaelweingartner commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Folks, what about a middle ground here?

I was checking the commits. For instance, all of the commits 
"Added/implemented XXX ." could be all squashed by the same author. There are a 
bunch of commits in this style that are introducing a single class. Also, 
subsequent commits that change the introduced classes by the same author can 
also be squashed. Therefore, no one loses merit and the history is maintained.

After the squashing process is done, we can evaluate and discuss the 
situation further. 



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[karuturi]

Github user karuturi commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
I was one of the RM for 4.6. We never made such a decision nor did we 
follow. There were multiple discussions on ML already about squashing vs 
merging and we never concluded to do squashing.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rhtyd]

Github user rhtyd commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
At least since August 2015, we've been following the guideline [1][2] for 
2LGTMs and to squash changes when they are accepted. AFAIK RMs for 4.6+ have 
been asking PR authors to squash their changes before merging the PR once their 
PR is accepted, the consensus has been followed and enforced.

I've only requested the PR authors to follow what has been followed in the 
community/PRs for a very long time, however I encourage any disagreements to be 
debated on dev ML than be discussed and burried in a pull request as this 
affects everyone. /cc @karuturi 

As an individual contributor, I prefer squashing changes for a single 
feature/FR into one or fewer commits. Sometimes, it may make sense to squash 
changes to more than one commit though 90 seems like an outrageous number. If 
multiple authors have worked on the PR, authors can consider squashing changes 
to lowest possible grouped/squashed changes. Lastly, we've also seen several 
feature PRs with several kloc+ changes that were accepted as a single squashed 
commit that may be searched through Github PR search page.

[1] http://markmail.org/thread/4cvxhm67ef356ppu
[2] 
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61311655


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[karuturi]

Github user karuturi commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
When debugging, I have seen issues with humongous commits with little or no 
history. They are very difficult to understand.
This is a feature and it need not be backported. so, backporting to 
previous releases is out of question. up-porting automatically happens.
If we want to revert the feature, we revert the merge which is very easy to 
do.
Also, reverting (for feature stability reasons) is just temporary. 
I agree that the number of commits is high. But, the amount of code is also 
high and from different authors who wants to keep it this way.
I am +1 with keeping it as is. 
I prefer merges with proper history and small logical units.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[priyankparihar]

Github user priyankparihar commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Hi @rhtyd,
>From my experience RM-ing for 4.3, 4.5, 4.9 -- the git history is pretty 
messed-up and it becomes far too difficult to track changes.

I think everything has its own + and -. Multiple commits  provide more 
insight and clarity on history. Could you please provide some more advantages 
of squashing with proper proof. 

>From my experience

This statement seems very loose. I think it needs some improvement. 



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rhtyd]

Github user rhtyd commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@rajesh-battala I won't accept this PR, sorry to share this but the number 
of commits are simply outrageous.

From my experience RM-ing for 4.3, 4.5, 4.9 -- the git history is pretty 
messed-up and it becomes far too difficult to track changes, backport/up-port 
commits/fixes/feature. There have been big PR/features that have been accepted 
and merged with very few overall squashed commits.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Current commits are in the order of some milestones.
As @nitin-maharan told if we squash them into one commit we loose
contributions of induviduals.


On Thu, Mar 16, 2017 at 7:09 PM, Rohit Yadav 
wrote:

> @nitin-maharana  it becomes easier to
> triage changes when changes are confined to a limited number of commits
> (ideally one per PR), please squash the commits based on the author (if 
not
> to a single commit) if you don't agree. Ideally, you can also group/squash
> commits based on the component/framework/architecture.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> ,
> or mute the thread
> 

> .
>



-- 
Thanks
Rajesh Battala



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rhtyd]

Github user rhtyd commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@nitin-maharana it becomes easier to triage changes when changes are 
confined to a limited number of commits (ideally one per PR), please squash the 
commits based on the author (if not to a single commit) if you don't agree. 
Ideally, you can also group/squash commits based on the 
component/framework/architecture.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@rafaelweingartner : As there are multiple contributors to this feature, If 
I squash it to one commit, then others are going to lose their part of 
contributions. Initially, we thought of making it to one commit, but this is 
the main reason we pushed with multiple commits. Let's wait for others to 
comment on this, after that we will decide. Thanks, @rafaelweingartner for 
pitching in.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rafaelweingartner]

Github user rafaelweingartner commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@nitin-maharana I was looking at the PR.
Do you need to split everything thing there in a different commit?
I think that we still do not have a clear understanding when and how to 
separate things in a commit; however, anything like 90+ commits in a PR seems 
exaggerated to me.

I like the philosophy that for every commit we should be able to get the 
system, build it and use it. This, for instance, would not happen here. Again, 
as I said, I think we do not have a clear rule about that, but I would like 
others to check this situation as well.

@DaanHoogland, @rhtyd, @swill any thoughts 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
It has already two LGTMs and contains all successful test results. This is 
a big change, as time passes there are more chances of conflicts 
appearance(Already resolved once). If anyone wants to review, please do it else 
we should consider merging 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Amazing effort.
From the test results We are good to merge to master.

Thansk
Rajesh Battala

On Wed, Mar 8, 2017 at 12:24 PM, Nitin Kumar Maharana <
notificati...@github.com> wrote:

> tag:mergeready
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> ,
> or mute the thread
> 

> .
>



-- 
Thanks
Rajesh Battala



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[sowmyakrishn]

Github user sowmyakrishn commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
LGTM based on following tests:

Tested with Automated runs for Shared and Dedicated Isolation policies - 
test_ncc_integration_dedicated.py  and test_ncc_integration_shared.py (also 
part of this PR) and both tests have passed.

Result:


test_01_shared_first_network 
(integration.component.test_ncc_integration_shared.TestNccIntegrationShared) 
... === TestName: test_01_shared_first_network | Status : SUCCESS ===
ok
test_02_shared_another_network 
(integration.component.test_ncc_integration_shared.TestNccIntegrationShared) 
... === TestName: test_02_shared_another_network | Status : SUCCESS ===
ok

--
Ran 2 tests in 472.393s

OK

test_01_dedicated_first_network 
(integration.component.test_ncc_integration_dedicated.TestNccIntegrationDedicated)
 ... === TestName: test_01_dedicated_first_network | Status : SUCCESS ===
ok
test_02_dedicated_another_network 
(integration.component.test_ncc_integration_dedicated.TestNccIntegrationDedicated)
 ... === TestName: test_02_dedicated_another_network | Status : SUCCESS ===
ok

--
Ran 2 tests in 234.199s

OK

Further, manual tests were also carried out successfully as per the Test 
Plan mentioned in 
https://cwiki.apache.org/confluence/display/CLOUDSTACK/NCC+Integration+with+CloudStack+Test+Plan



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Ping @sowmyakrishn @jayapalu 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
I have accepted the patch you can push it to master


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[cloudmonger]

Github user cloudmonger commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
### ACS CI BVT Run
 **Sumarry:**
 Build Number 439
 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@rajesh-battala : Currently running, will post once completes.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Please find the test plan executed from below link(Updated in wiki page).

https://cwiki.apache.org/confluence/display/CLOUDSTACK/NCC+Integration+with+CloudStack+Test+Plan



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Thanks.
Once its update please point the same here. So that every one knows it.

Thanks
Rajesh Battala

On Tue, Mar 7, 2017 at 9:27 AM, Nitin Kumar Maharana <
notificati...@github.com> wrote:

> @rajesh-battala  : Will update the
> same in wiki page.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> ,
> or mute the thread
> 

> .
>



-- 
Thanks
Rajesh Battala



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@rajesh-battala : Will update the same in wiki page. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Can you please share the test cases and Test plan executed for this feature.
It would be great if community knows the testing done on this feature and 
its coverage.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
ping @rajesh-battala @sowmyakrishn 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Sure @rajesh-battala, will look at 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[rajesh-battala]

Github user rajesh-battala commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
@nitin-maharana @sowmyakrishn  Can you please check why CI tests are 
failing?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack issue #1859: CLOUDSTACK-8672 : NCC Integration with CloudStack

[nitin-maharana]

Github user nitin-maharana commented on the issue:

https://github.com/apache/cloudstack/pull/1859
  
Hi @rajesh-battala, I have modified the code. Please review it once and let 
me know if any improvement required. 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---