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

Python 3 updates #280

Merged
merged 55 commits into from
Nov 27, 2019
Merged

Python 3 updates #280

merged 55 commits into from
Nov 27, 2019

Conversation

borrob
Copy link
Collaborator

@borrob borrob commented Sep 17, 2019

Please do NOT approve this pull request. This is Work In Progress for the python3 conversion. I have an issue with one specific test and we want to see if Travis gets the same error or if it passes without problems.

@justb4
Copy link
Member

justb4 commented Sep 19, 2019

The Travis failure is only a flake8 error.

@borrob
Copy link
Collaborator Author

borrob commented Sep 19, 2019

:) yes, so the normal testing passes, so I guess it is something with my setup that the one test fails on my machine.

I haven't yet had much time to work on the docker issue, but otherwise I would say we are almost there.

@borrob
Copy link
Collaborator Author

borrob commented Oct 16, 2019

Weird... I would expect SQLAlchemy to deal with the abstraction and get the string/byte conversion right regardless of which database is used. I will look into it.

Agree: we should be able to drop six.

From code review by @justb4:
6 - Auth info Decoding/Encoding Failure with Postgres

Only occurs when using Postgres, not with SQLite. Background: GHC
encodes/encrypts a generic auth dict info structure via JSON string to be stored
as textfield in DB. It decodes/decrypts when reading. This way we can support
multiple auth types with a single auth column in resource table. So this is
another encoding than in 4 for HTTP auth headers, but think similar problem.
We also need to deal with existing PG DBs that have auth columns already present
in resource table.

-  install psycopg2 : pip install psycopg2
-  used existing PG DB, only changed in config_site.py: SQLALCHEMY_DATABASE_URI
   = 'postgresql://name:passw@localhost:5432/ghc', but may create new
-  add URL Resource with Basic Auth (even does not have to have basic auth)
-  in Edit add Basic Auth Username and Password
-  click Save

Fixed by ensuring encode/decode takes `string` as input and gives `string` as
output. These methodes now use static typing. The origin of the problem was the
difference of how SQLite and Postgres store the encoded string (either as text
or in some binary form). This solution makes the encoded authentication a
string object and *not* bytes.
@borrob
Copy link
Collaborator Author

borrob commented Oct 21, 2019

We're not there yet: I noticed some paver commands still need an update because of issues with the importing of modules. Also: the docker image is building, but I haven't checked yet if it is actually working. I had to change the gunicorn configuration on a non-docker deploy.

@justb4
Copy link
Member

justb4 commented Oct 25, 2019

@borrob good to see you synced with master! Will try not to do too many big changes.

The Postgres-char-issue: my bad! For security reasons the SECRET_KEY is used for encoding/decoding stored auth creds, and the Py3 instance had a different key! Using the same key: no problem. Pff, was thinking that we had a very serious encoding issue with difficult DB migrations. So we're getting closer!

@borrob
Copy link
Collaborator Author

borrob commented Oct 25, 2019

I fixed one paver issue and did some testing (also with docker). I think we're good to go and I'm curious what the results of the demo environment will be.

@borrob borrob marked this pull request as ready for review October 25, 2019 19:38
@borrob
Copy link
Collaborator Author

borrob commented Oct 25, 2019

I removed the draft tag from this pull request (that was there for the automated testing with travis). Please review and let's hope we can move to py3 soon!

@justb4
Copy link
Member

justb4 commented Oct 27, 2019

Yes, good, I only want to release 0.7.0 from current master first, and create a 07.0 maintenance branch. Then further test in particular with Docker and then merge this PR and have a quite some testing time on demo site. OK, @tomkralidis ? Let's aim for all of this before nov 1 ok?

NB solved a nasty concurrency bug (two lines) with #301 #302 today.

@@ -61,7 +61,7 @@ def db_commit():
err = None
try:
DB.session.commit()
except Exception as err:
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are here, for consistency, can we log the error,

            except Exception as err:
                LOGGER.warning("Cannot commit to database %s".format(err))

@@ -1,5 +1,4 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to remove this shebang line entirely.

@@ -228,22 +227,24 @@ def read(filename, encoding='utf-8'):


# https://gist.github.com/gowhari/fea9c559f08a310e5cfd62978bc86a1a
def encode(key, string):
# with alterations to get return type of string, not bytes
def encode(key: str, string: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this annotation specific to Python 3.7+ or safe for 3.4+, say?

@tomkralidis tomkralidis self-requested a review November 27, 2019 01:30
Copy link
Member

@justb4 justb4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did verifications with latest from borrob's branch:

  • SQLite and Postgres backends: tests all run ok.
  • Running from commandline ok
  • build/run with Docker ok.

So we're good to go! Let's all check on demo.geohealthcheck.org to further verify.

@tomkralidis tomkralidis changed the title WIP Py3 Python 3 updates Nov 27, 2019
@tomkralidis tomkralidis merged commit 046c6b0 into geopython:master Nov 27, 2019
@justb4
Copy link
Member

justb4 commented Nov 27, 2019

A great thank-you @borrob! This was no easy PR to do.

@tomkralidis
Copy link
Member

Thanks for the awesome contribution @borrob!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants