-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add database schema generation #603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #603 +/- ##
==========================================
+ Coverage 90.01% 90.02% +<.01%
==========================================
Files 55 55
Lines 2645 2646 +1
Branches 341 341
==========================================
+ Hits 2381 2382 +1
Misses 201 201
Partials 63 63
Continue to review full report at Codecov.
|
9675c79
to
f5c98be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of my comments are blockers, though I do strongly recommend making the upstream project use the explicit shebang line.
@@ -0,0 +1,22 @@ | |||
#!/usr/bin/env python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that Fedora's Python packaging guidelines will require this to be changed to an explicit Python version (and not to use env too), like /usr/bin/python3
. It is possible to use sed
in the specfile to accomplish this, but I recommend using an explicit shebang here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anitya isn't yet packaged in Fedora, but thanks for pointing at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried with /usr/bin/python3
and this isn't working. Probably because of the sqlalchemy_schemadisplay
is python2 import.
However it's working with /usr/bin/env python3
.
from anitya.db import meta | ||
|
||
|
||
def write_graph(filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to add a docblock here, including the type of the filename
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this from Bodhi, changed only what is imported. :-)
@@ -19,6 +19,7 @@ | |||
- python3-devel | |||
- redhat-rpm-config | |||
- rpm-python | |||
- python2-sqlalchemy_schemadisplay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use the python3
package instead, or is Anitya not yet Python 3 ready? Python 2 is deprecated in Fedora 30 and some packages are getting dropped there (not sure about schemadisplay).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look if there is python3
package for this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there isn't python3
package in Fedora 28.
However I see that the package maitainer is @jeremycline - Do you plan python3 package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into it. Not sure why I didn't do a Python 3 package to begin with... Maybe it didn't support Python 3? I guess I'll find out
heh, I forgot about Mergify. Well if you make another PR in light of my recommendation, let me know and I'll review ☺ |
Yes, I will do. Thanks for review. |
Moved to separate PR from #601 as new feature.