Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLOUDSTACK-10024: Network migration support #2259

Merged
merged 1 commit into from Dec 21, 2017

Conversation

@sgoeminn
Copy link
Contributor

commented Sep 4, 2017

@sgoeminn sgoeminn changed the title PROD-4805: isolated and vpc network migration from one physical netwo… CLOUDSTACK-10024: isolated and vpc network migration from one physical netwo… Sep 4, 2017
@sgoeminn sgoeminn changed the title CLOUDSTACK-10024: isolated and vpc network migration from one physical netwo… CLOUDSTACK-10024: isolated and vpc network migration from one physical network to another physical network Sep 4, 2017
@sgoeminn sgoeminn changed the title CLOUDSTACK-10024: isolated and vpc network migration from one physical network to another physical network CLOUDSTACK-10024: Network migration support Sep 4, 2017
@sgoeminn sgoeminn force-pushed the nuagenetworks:feature/migration_upstream branch 3 times, most recently from 71fab71 to 21f2224 Sep 4, 2017
@sgoeminn sgoeminn force-pushed the nuagenetworks:feature/migration_upstream branch 2 times, most recently from 273d627 to 20088e9 Sep 20, 2017
@sgoeminn sgoeminn force-pushed the nuagenetworks:feature/migration_upstream branch 4 times, most recently from 5446140 to 51cd6bd Oct 20, 2017
Copy link
Contributor

left a comment

Verify Migration for an isolated network without VMs ... === TestName: test_01_native_to_nuage_network_migration_novms | Status : SUCCESS ===
ok
Verify Migration for an isolated network with stopped VMs ... === TestName: test_02_native_to_nuage_network_migration_stoppedvms | Status : SUCCESS ===
ok
Verify traffic after Migration of a non-persistent isolated network ... === TestName: test_03_migrate_native_nonpersistent_network_to_nuage_traffic | Status : SUCCESS ===
ok
Verify traffic after Migration of a persistent isolated network ... === TestName: test_04_migrate_native_persistentnetwork_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in an isolated network ... === TestName: test_05_native_to_nuage_nic_migration | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a non-persistent isolated network ... === TestName: test_06_migrate_native_nonpersistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a persistent isolated network ... === TestName: test_07_migrate_native_persistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm with multiple isolated networks ... === TestName: test_08_migrate_native_multinic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a persistent isolated network ... === TestName: test_09_migrate_native_persist_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
test_10_migrate_native_vpc (nuagevsp.test_nuage_network_migration.TestNuageMigration) ... === TestName: test_10_migrate_native_vpc | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a vpc network ... === TestName: test_11_migrate_native_vpc_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm in multiple networks ... === TestName: test_12_migrate_native_vpc_multinic_to_nuage_traffic | Status : SUCCESS ===
ok


Ran 12 tests in 11427.939s

OK

Copy link
Member

left a comment

Here goes the first batch of raised points. I will try to review it again on this week, lots of lines to analyze yet; feel free to discuss and send questions meanwhile.

Thanks for the hard work @sgoeminn 👍

* @param networkOfferingId
* @param callerAccount
* @param callerUser
* @return

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Thanks for documenting the code @sgoeminn.
Please, remove the @param and @return. I don't see much use of them (unless they are detailed). The same goes for method 'migrateVpcNetwork'.

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

Updated the documentation.

import com.cloud.user.Account;
import com.cloud.user.User;

@APICommand(name = "migrateNetwork", description = "moves a network do another physical network", responseObject = NetworkResponse.class, responseView = ResponseView.Restricted, entityType = {Network.class},

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

description = "moves a network do another physical network" -> moves a network to another

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

I fixed the typo.

}

@Override
public void execute() throws InsufficientCapacityException, ConcurrentOperationException {

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Sorry if I am missing something here, but I do not see where those exceptions are launched; is it necessary to add the throws InsufficientCapacityException, ConcurrentOperationException? The compiler doesn't complain when removing them.

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

You are right, they are not necessary. We removed them.

}

@Override
public void execute() throws InsufficientCapacityException, ConcurrentOperationException {

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Are these exceptions needed?

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

You are right, they are not necessary. We removed them.

// 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.
///

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Can you please change the licence header?

Expected:

// 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.

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

That's indeed the wrong license. We changed it to the correct one. Thanks!

* @param account
* @param callerUser
* @param resume
* @return

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Can you please remove the @param annotations or provide a description?

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

Updated the documentation.

* @param resourceId
* @param resourceType
* @param key
* @return

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Can you please remove the @param annotations or provide a description?

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

Updated the documentation.

* remove a resource tag based on the resourceid, resourtype and key
* @param resourceId
* @param resourceType
* @param key

This comment has been minimized.

Copy link
@GabrielBrascher

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

Updated the documentation.

@@ -1196,6 +1222,10 @@ private void getPifs() {
s_logger.debug("done looking for pifs, no more bridges");
}

boolean isGuestBridge(String bridge) {

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Thanks! Removing the duplicate code is great 👍
If you want, you can go a step further creating a test case for isGuestBridge(String bridge) and isPublicBridge(String bridge) ;)

void deleteCopyOfNetwork(long networkCopyId, long originalNetworkId);

/**
*

This comment has been minimized.

Copy link
@GabrielBrascher

GabrielBrascher Oct 23, 2017

Member

Missing documentation.

This comment has been minimized.

Copy link
@sgoeminn

sgoeminn Oct 24, 2017

Author Contributor

Updated the documentation.

@sgoeminn sgoeminn force-pushed the nuagenetworks:feature/migration_upstream branch from 51cd6bd to b6951ee Oct 24, 2017
@sgoeminn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2017

@GabrielBrascher Thanks for reviewing, really appreciate it! We tried to address all your comments.

@sgoeminn sgoeminn force-pushed the nuagenetworks:feature/migration_upstream branch 2 times, most recently from dc02888 to 8ef5a2c Nov 30, 2017
@smeetsr

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

Verify Migration for an isolated network without VMs ... === TestName: test_01_native_to_nuage_network_migration_novms | Status : SUCCESS ===
ok
Verify Migration for an isolated network with stopped VMs ... === TestName: test_02_native_to_nuage_network_migration_stoppedvms | Status : SUCCESS ===
ok
Verify traffic after Migration of a non-persistent isolated network ... === TestName: test_03_migrate_native_nonpersistent_network_to_nuage_traffic | Status : SUCCESS ===
ok
Verify traffic after Migration of a persistent isolated network ... === TestName: test_04_migrate_native_persistentnetwork_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in an isolated network ... === TestName: test_05_native_to_nuage_nic_migration | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a non-persistent isolated network ... === TestName: test_06_migrate_native_nonpersistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a persistent isolated network ... === TestName: test_07_migrate_native_persistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm with multiple isolated networks ... === TestName: test_08_migrate_native_multinic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a persistent isolated network ... === TestName: test_09_migrate_native_persist_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
test_10_migrate_native_vpc (nuagevsp.test_nuage_network_migration.TestNuageMigration) ... === TestName: test_10_migrate_native_vpc | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a vpc network ... === TestName: test_11_migrate_native_vpc_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm in multiple networks ... === TestName: test_12_migrate_native_vpc_multinic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify migration of GuestVm with ip .2 still works ... === TestName: test_13_verify_guestvmip2_when_migrating_to_nuage | Status : SUCCESS ===
ok


Ran 13 tests in 15064.370s

OK
runinfo.txt

@sgoeminn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

@DaanHoogland @borisstoyanov is it possible to run CI against this PR?

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

@sgoeminn can you please remind me towards the end of the week, we're running out of resources in Trillian and need to prioritize at this moment. If we start the smoketests on Fri/Sat, you'll be able to see them on Monday.

@sgoeminn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2017

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2017

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 8, 2017

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Copy link
Contributor

left a comment

Can we have tests with default network provider for the new APIs? I see we have quite a bunch with nuage but none with the default one.

@sgoeminn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2017

@borisstoyanov We already have tests that migrate from Nuage to Native (and visa versa). But we will add some new tests which do migration from native physical network A to native physical network B.

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 8, 2017

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1355

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2017

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 9, 2017

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@krissterckx

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@borisstoyanov this PR adds MigrateNetworkCmd.class and MigrateVPCCmd.class as new commands, which just causes git not to know in which order to add them.
The problem is, we can keep on rebasing - each time some other commits goes in, we possibly need to rebase again :-) We can/will do that but i also would like to see review and Trillian results. Do you prefer us to rebase again first ?

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@krissterckx sure I can kick the marvin tests, have you managed to add the default network provider tests?

Co-Authored-By: Frank Maximus frank.maximus@nuagenetworks.net
Co-Authored-By: Raf Smeets raf.smeets@nuagenetworks.net

New API’s:

* migrateNetwork
* migrateVpc
@fmaximus fmaximus force-pushed the nuagenetworks:feature/migration_upstream branch from 7b51b32 to 8dddbb1 Dec 20, 2017
@krissterckx

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Hi @borisstoyanov , see https://github.com/nuagenetworks/cloudstack/blob/8dddbb1544cbd34e68e772c294e0e3e287c06f0b/test/integration/plugins/nuagevsp/test_nuage_network_migration.py#L1859
and https://github.com/nuagenetworks/cloudstack/blob/7b51b325bbb85a6bb989dd6b18814ceac26da25a/test/integration/plugins/nuagevsp/test_nuage_network_migration.py#L1902

They migrate an isolated network and VPC within a native physical network, to a new offering. It is a special case of the migration across physical networks, but it comes down to the same behavior : the whole network is reimplemented again as the offering is different. We don't have the infrastructure set up for setting up multiple native physical networks, hence we took this approach.

Is it ok we left this test under this nuage test module ? Or would you like to see a new native component test. Let me know what you think , i.e. whether what we did is fine or it is not what you had in mind.
Thanks.

Copy link
Contributor

left a comment

LGTM

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@krissterckx Yes, that should be fine. Let me run some tests.
@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 20, 2017

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Can you paste some execution logs for the newly added tests, since they won't be picked up from the plugin directory.

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 20, 2017

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1441

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 20, 2017

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@smeetsr

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Verify Migration for an isolated network without VMs ... === TestName: test_01_native_to_nuage_network_migration_novms | Status : SUCCESS ===
ok
Verify Migration for an isolated network with stopped VMs ... === TestName: test_02_native_to_nuage_network_migration_stoppedvms | Status : SUCCESS ===
ok
Verify traffic after Migration of a non-persistent isolated network ... === TestName: test_03_migrate_native_nonpersistent_network_to_nuage_traffic | Status : FAILED ===
FAIL due to testbedissue - rerun of this test was fine.
Verify traffic after Migration of a persistent isolated network ... === TestName: test_04_migrate_native_persistentnetwork_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in an isolated network ... === TestName: test_05_native_to_nuage_nic_migration | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a non-persistent isolated network ... === TestName: test_06_migrate_native_nonpersistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify Nic migration of GuestVm in a persistent isolated network ... === TestName: test_07_migrate_native_persistent_nic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm with multiple isolated networks ... === TestName: test_08_migrate_native_multinic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a persistent isolated network ... === TestName: test_09_migrate_native_persist_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
test_10_migrate_native_vpc (nuagevsp.test_nuage_network_migration.TestNuageMigration) ... === TestName: test_10_migrate_native_vpc | Status : SUCCESS ===
ok
Verify StaticNat migration of GuestVm in a vpc network ... === TestName: test_11_migrate_native_vpc_staticnat_to_nuage_traffic | Status : SUCCESS ===
ok
Verify MultiNic migration of GuestVm in multiple networks ... === TestName: test_12_migrate_native_vpc_multinic_to_nuage_traffic | Status : SUCCESS ===
ok
Verify migration of GuestVm with ip .2 still works ... === TestName: test_13_verify_guestvmip2_when_migrating_to_nuage | Status : SUCCESS ===
ok
Verify Migration for an isolated network nativeOnly ... === TestName: test_14_native_to_native_network_migration | Status : SUCCESS ===
ok
Verify Migration for a vpc network nativeOnly ... === TestName: test_15_native_to_native_vpc_migration | Status : SUCCESS ===
ok

runinfo.txt

@krissterckx

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@borisstoyanov , see above post :

Verify Migration for an isolated network nativeOnly ... === TestName: test_14_native_to_native_network_migration | Status : SUCCESS ===
ok
Verify Migration for a vpc network nativeOnly ... === TestName: test_15_native_to_native_vpc_migration | Status : SUCCESS ===
ok

thank, Kris

@blueorangutan

This comment has been minimized.

Copy link

commented Dec 21, 2017

Trillian test result (tid-1847)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34856 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2259-t1847-kvm-centos7.zip
Smoke tests completed. 61 look OK, 6 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestDeployVirtioSCSIVM>:teardown Error 50.73 test_deploy_virtio_scsi_vm.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 Failure 255.03 test_internal_lb.py
test_01_vpc_privategw_acl Failure 86.85 test_privategw_acl.py
test_02_vpc_privategw_static_routes Failure 238.53 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 198.16 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 334.25 test_privategw_acl.py
test_02_create_template_with_checksum_sha1 Error 5.21 test_templates.py
test_03_create_template_with_checksum_sha256 Error 5.22 test_templates.py
test_04_create_template_with_checksum_md5 Error 5.26 test_templates.py
test_01_vpc_remote_access_vpn Error 55.90 test_vpc_vpn.py
test_02_cancel_host_maintenace_with_migration_jobs Error 202.12 test_host_maintenance.py
@krissterckx

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@borisstoyanov based on these results, are we good to merge ? i have less insight in which are known issues.

Copy link
Contributor

left a comment

Yes @krissterckx, LGTM

@fmaximus fmaximus merged commit d497656 into apache:master Dec 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
public class MigrateNetworkCmd extends BaseAsyncCmd {
public static final Logger s_logger = Logger.getLogger(MigrateNetworkCmd.class.getName());

private static final String s_name = "migratenetworkresponse";

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

Please follow recent API implementation patterns, see RoleCmd as an example. The API name and response names can be refactored.

import com.cloud.user.User;

@APICommand(name = "migrateNetwork", description = "moves a network to another physical network", responseObject = NetworkResponse.class, responseView = ResponseView.Restricted, entityType = {Network.class},
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

There is no default authorized defined, please fix.

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

The API does no define a since as well.

import com.cloud.user.User;

@APICommand(name = "migrateVPC", description = "moves a vpc to another physical network", responseObject = VpcResponse.class, responseView = ResponseObject.ResponseView.Restricted, entityType = {Vpc.class},
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

There is no default authorized defined, please fix.

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

The API does no define a since as well.

public class MigrateVPCCmd extends BaseAsyncCmd {
public static final Logger s_logger = Logger.getLogger(MigrateVPCCmd.class.getName());

private static final String s_name = "migratevpcresponse";

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

Same as above, refactor to use APINAME etc patterns as used in recent APIs.

return flatMap;
}

for (HashMap<String, String> map : tierNetworkOfferings.values()) {

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

a putall could be used here.

vm.updateDeviceFlags(interfaceDef.toString(), DomainAffect.LIVE.getValue());

/*
// Manual replug

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

Dead code could be removed.

@@ -387,9 +407,10 @@
"displaytext": "MySharedOffering",
"guestiptype": "Shared",
"supportedservices": "Dhcp,Dns,UserData",
"specifyVlan": "False",
"specifyIpRanges": "False",
"specifyVlan": "True",

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

Changing existing test data may cause failures for other tests.

"serviceCapabilityList": {
"SourceNat": {"SupportedSourceNatTypes": "perzone"}
}
},

This comment has been minimized.

Copy link
@rhtyd

rhtyd Dec 21, 2017

Member

If the test data is specific to Nuage specific marvin tests, these can be moved as Service in the tests themselves.

@rhtyd

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

@sgoeminn @krissterckx @fmaximus kindly send a new PR to fix additional refactorations especially around APIs as they go with the release's apidocs. If the new APIs are general purpose, please add new marvin tests.

I also see a new regression/failure in test_internal_lb.py, please check. Given we're weeks away from 4.11 cut/freezing, please ask additional code reviews on features by non-colleagues. Thanks.

@krissterckx

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@rhtyd thanks for review; will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.