fgpv-vpgf / rcs

RAMP Configuration Service
http://fgpv-vpgf.github.io/rcs
1 stars 8 forks source link

python 3.6 migration #85

Closed barryytm closed 6 years ago

barryytm commented 6 years ago

Closes #79

Link to test: http://section917.cloudapp.net:8080/static/test.html

JSON sample snippet for testing:

{  
   "version":"2.0",
   "fr":{  
      "service_url":"https://agrimaps.gov.mb.ca/arcgis/rest/services/AGRIMAPS/AGRIMAPS/MapServer/3",
      "service_type":"esriFeature",
      "service_name":"Trunk Highways du Provincial"
   },
   "en":{  
      "service_url":"https://agrimaps.gov.mb.ca/arcgis/rest/services/AGRIMAPS/AGRIMAPS/MapServer/3",
      "service_type":"esriFeature",
      "service_name":"Provincial Trunk Highways"
   }
}

This change is Reviewable

alyec commented 6 years ago

Reviewed 27 of 27 files at r1. Review status: all files reviewed at latest revision, 6 unresolved discussions.


requirements.txt, line 4 at r1 (raw file):

Flask-RESTful==0.3.6
jsonschema==2.6.0
requests==2.2.1

why did requests move to such an old version?


Vagrantfile, line 47 at r1 (raw file):


  config.vm.provision "shell", inline: <<-SHELL
    sudo add-apt-repository ppa:deadsnakes/ppa

instead of using a ppa it would be better to update the base OS version, ubuntu 14.04 is only a year away from dropping support

when looking for new vagrant images it's probably better to look for an image which is supported by vagrant


Vagrantfile, line 56 at r1 (raw file):

    curl -X PUT http://127.0.0.1:5984/rcs_auth
    cd /vagrant
    virtualenv --always-copy -p python3.6 .

please use the new venv module with python 3 (this should also remove the need for python-virtualenv as a package)


services/upgrade.py, line 53 at r1 (raw file):

            upgrade_method = wms_upgrade if v1_request['payload_type'] == 'wms' else feat_upgrade
            v2_request = {lang: upgrade_method(v1_request[lang]) for lang in current_app.config['LANGS']}
            print(v2_request)

this should probably get converted to a log if we want to keep it, and if not delete it


services/regparse/universal.py, line 56 at r1 (raw file):

            endpoint += '?VERSION=1.1.1&REQUEST=GetCapabilities&SERVICE=wms'
        r = requests.get(endpoint)
        print(r.status_code)

print statements should become logs or be removed, these are probably left over from debugging sessions


test/init.py, line 0 at r1 (raw file): why is test becoming a package?


Comments from Reviewable

james-rae commented 6 years ago

Reviewed 22 of 27 files at r1, 7 of 7 files at r2. Review status: all files reviewed at latest revision, 7 unresolved discussions.


setup.py, line 10 at r2 (raw file):

    name="rcs",
    description="RAMP Configuration Service",
    version="2.4.0",

would a change this large be a v3.0.0? @alyec ?


Comments from Reviewable

alyec commented 6 years ago

Reviewed 6 of 7 files at r2. Review status: all files reviewed at latest revision, 7 unresolved discussions.


setup.py, line 10 at r2 (raw file):

Previously, james-rae (James Rae) wrote…
would a change this large be a `v3.0.0`? @alyec ?

when we add a "from future import ..." that generally means the app can work in python2 as well as python3 https://github.com/fgpv-vpgf/rcs/blob/develop/run.py#L5

the sigcheck module may break and need custom code to run in 2 as it is now

given the scope of the changes I'd be willing to go with a 2.4 or a 3.0 as I'm not entirely clear on where something like this stands

if we're dropping support for all python2 installs I'd say go with 3.0


Vagrantfile, line 51 at r2 (raw file):

    netstat -antp
    sleep 5
    curl -X DELETE http://localhost:5984/rcs_cache

why are these deletes needed?


Vagrantfile, line 57 at r2 (raw file):

    cd /vagrant
    python3.6 -m venv rcs-venv
    . rcs-venv/bin/activate

what's the reason for moving the venv into a separate directory?


Comments from Reviewable

barryytm commented 6 years ago

Review status: 21 of 27 files reviewed at latest revision, 9 unresolved discussions.


setup.py, line 10 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote…
when we add a "from __future__ import ..." that generally means the app can work in python2 as well as python3 https://github.com/fgpv-vpgf/rcs/blob/develop/run.py#L5 the sigcheck module may break and need custom code to run in 2 as it is now given the scope of the changes I'd be willing to go with a 2.4 or a 3.0 as I'm not entirely clear on where something like this stands if we're dropping support for all python2 installs I'd say go with 3.0

Changed to 3.0


Vagrantfile, line 47 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
instead of using a ppa it would be better to update the base OS version, ubuntu 14.04 is only a year away from dropping support when looking for new vagrant images it's probably better to look for an image which is supported by vagrant

Changed to 16.04, thanks.


Vagrantfile, line 56 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
please use the new venv module with python 3 (this should also remove the need for python-virtualenv as a package)

Done, thanks.


Vagrantfile, line 51 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote…
why are these deletes needed?

Removed, thanks.


Vagrantfile, line 57 at r2 (raw file):

Previously, alyec (Aly Merchant) wrote…
what's the reason for moving the venv into a separate directory?

Moved back to the same directory, thanks.


services/upgrade.py, line 53 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
this should probably get converted to a log if we want to keep it, and if not delete it

Removed, thanks.


services/regparse/universal.py, line 56 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
print statements should become logs or be removed, these are probably left over from debugging sessions

Removed, thanks.


requirements.txt, line 4 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
why did requests move to such an old version?

Thought it was newer. Reverted back, thanks.


test/init.py, line at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
why is test becoming a package?

Removed, thanks.


Comments from Reviewable

alyec commented 6 years ago

Reviewed 7 of 7 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.


Vagrantfile, line 47 at r1 (raw file):

Previously, barryytm (Barry Yung) wrote…
Changed to 16.04, thanks.

please check bento/ubuntu-16.04 as if that's usable then we're better off going with that image

also, which version of couchdb is being installed?


Comments from Reviewable

barryytm commented 6 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.


Vagrantfile, line 47 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
please check `bento/ubuntu-16.04` as if that's usable then we're better off going with that image also, which version of couchdb is being installed?

I wasn't able to install couchdb on bento/ubuntu-16.04. The current installed version of couchdb is 1.6.0.


Comments from Reviewable

alyec commented 6 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.


Vagrantfile, line 47 at r1 (raw file):

Previously, barryytm (Barry Yung) wrote…
I wasn't able to install couchdb on `bento/ubuntu-16.04`. The current installed version of couchdb is 1.6.0.

Can you try the 2.x line installation source from the apache repos at http://docs.couchdb.org/en/2.1.1/install/unix.html#installation-using-the-apache-couchdb-convenience-binary-packages ? Try this on bento/ubuntu-16.04 and see how it goes.


Comments from Reviewable

james-rae commented 6 years ago

Reviewed 6 of 7 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

barryytm commented 6 years ago

Reviewed 19 of 27 files at r1, 2 of 7 files at r2, 6 of 7 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Vagrantfile, line 47 at r1 (raw file):

Previously, alyec (Aly Merchant) wrote…
Can you try the 2.x line installation source from the apache repos at http://docs.couchdb.org/en/2.1.1/install/unix.html#installation-using-the-apache-couchdb-convenience-binary-packages ? Try this on `bento/ubuntu-16.04` and see how it goes.

CouchDB 2.x requires a configuration file. Will keep it at 1.6.0 for now.


Comments from Reviewable

alyec commented 6 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


Vagrantfile, line 47 at r1 (raw file):

Previously, barryytm (Barry Yung) wrote…
CouchDB 2.x requires a configuration file. Will keep it at 1.6.0 for now.

To clarify, it's not due to the config file but rather that it doesn't come with the xenial image, and no one wants to spend enough time to install from source. We may also end up changing from vagrant to docker for the development platform.


Comments from Reviewable