add a user database #3

Merged
Ghost merged 6 commits from user-db into main 2022-10-26 14:05:48 +02:00
First-time contributor

This PR aims to add a user database to to the git importer and enable import and batch import of users from the OBS

This PR aims to add a user database to to the git importer and enable import and batch import of users from the OBS
Ghost added 4 commits 2022-10-26 10:28:25 +02:00
Ghost requested changes 2022-10-26 10:34:58 +02:00
lib/importer.py Outdated
@ -11,2 +11,4 @@
from lib.proxy_sha256 import ProxySHA256, md5, sha256
from lib.request import Request
import xml.etree.ElementTree as ET
from lib.users import User
Author
First-time contributor

Please rename that to lib.user

Please rename that to lib.user
Ghost marked this conversation as resolved
lib/importer.py Outdated
@ -159,2 +161,4 @@
self.update_db_package(db, lproject, lpackage)
missing_users = User.get_batch(db)
print(missing_users)
Author
First-time contributor

print left

print left
Ghost marked this conversation as resolved
lib/importer.py Outdated
@ -161,0 +167,4 @@
if missing_user is not None:
missing_user.import_into_db(db)
else:
logging.info("No missing users")
Author
First-time contributor

That else is strange. If there are no missing users, I would expect get_batch returns an empty list and you don't even end up in the for loop's body

That else is strange. If there are no missing users, I would expect get_batch returns an empty list and you don't even end up in the for loop's body
Owner

Indeed.

IMHO avoid is None, == []. Use the truth value (if missing_user:)

Indeed. IMHO avoid `is None`, `== []`. Use the truth value (`if missing_user:`)
Ghost marked this conversation as resolved
lib/importer.py Outdated
@ -173,6 +184,10 @@ class Importer:
else:
rev.set_broken(db)
# fake_accounts = ["unknown","buildservice-autocommit", "autobuild", "_service"]
Author
First-time contributor

Dead code?

Dead code?
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +5,4 @@
self.userid = userid
self.realname = xml.find("realname").text
self.email = xml.find("email").text
if self.email == None:
Author
First-time contributor

is None

is None
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +20,4 @@
cur = db.cursor()
cur.execute(
"""INSERT INTO users (userid, realname, email)
VALUES (%s,%s,%s) RETURNING id""",
Author
First-time contributor

if you don't need the id, don't have it RETURNING

if you don't need the id, don't have it RETURNING
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +29,4 @@
)
cur.close()
def lookup(db, userid):
Author
First-time contributor

this function is part of the dead code

this function is part of the dead code
Author
First-time contributor

I think there is value in keeping this for now to look up specific users

I think there is value in keeping this for now to look up specific users
Author
First-time contributor

Then it needs to be a class method

Then it needs to be a class method
Owner

@staticmethod

`@staticmethod`
Author
First-time contributor

Classmethod or Staticmethod? I would have defaulted to classmethod but I can change it either way

Classmethod or Staticmethod? I would have defaulted to classmethod but I can change it either way
Owner

@staticmethod, those are method associated with a class and not an instance. @classmethod are use for metaprogramming, when you want to change the class definition (weird stuff).

In a different comment I linked a bit about this.

`@staticmethod`, those are method associated with a class and not an instance. `@classmethod` are use for metaprogramming, when you want to change the class definition (weird stuff). In a different comment I linked a bit about this.
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +39,4 @@
return row
@classmethod
def get_batch(self, db):
Author
First-time contributor

Make that missing_users.

Make that missing_users.
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +48,4 @@
('unknown','buildservice-autocommit', 'autobuild', '_service')"""
)
missing_users = [row[0] for row in cur.fetchall()]
if not missing_users:
Author
First-time contributor

always close the cursor and just return missing_users. It's always a list, possibly an empty one - which you can return when there are no missing_users

always close the cursor and just return missing_users. It's always a list, possibly an empty one - which you can return when there are no missing_users
Ghost marked this conversation as resolved
aplanas requested changes 2022-10-26 10:47:45 +02:00
lib/importer.py Outdated
@ -176,0 +187,4 @@
# fake_accounts = ["unknown","buildservice-autocommit", "autobuild", "_service"]
# if User.user_lookup(db, rev.userid) is None and rev.userid not in fake_accounts:
# self.obs.users(rev.userid).import_into_db(db)
Owner

Remove this section?

Remove this section?
Ghost marked this conversation as resolved
@ -125,0 +134,4 @@
def user(self, userid):
root = self._user(userid)
if root is not None:
return User().parse(root, userid)
Owner

It is OK how you write it, but I would expect root and userid as part of the __init__ constructor (give initial state).

It is OK how you write it, but I would expect `root` and `userid` as part of the `__init__` constructor (give initial state).
Author
First-time contributor

root not as it's OBS specific while we want to have Users also coming from the DB

root not as it's OBS specific while we want to have Users also coming from the DB
aplanas marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +1,55 @@
FAKE_ACCOUNTS = ('unknown','buildservice-autocommit', 'autobuild', '_service')
Owner

For 4 elements a tuple is totally OK. If you expect that it will grow, use a set.

Missing space between the first and second element (I recommend you to use black to normalize the style)

For 4 elements a tuple is totally OK. If you expect that it will grow, use a `set`. Missing space between the first and second element (I recommend you to use `black` to normalize the style)
Author
First-time contributor

I actually added a Makefile so make all runs all the formaters I use :)

I actually added a Makefile so `make all` runs all the formaters I use :)
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +5,4 @@
self.userid = userid
self.realname = xml.find("realname").text
self.email = xml.find("email").text
if self.email == None:
Owner

is, black will detect this kind of issues too (I think)

`is`, `black` will detect this kind of issues too (I think)
Ghost marked this conversation as resolved
lib/users.py Outdated
@ -0,0 +39,4 @@
return row
@classmethod
def get_batch(self, db):
Owner

If it is a class method do not expect self as a first parameter, it is usually cls [1]

Besides, I think that you whant @staticmethod here[2], in that case drop self

[1] https://docs.python.org/3/library/functions.html#classmethod
[2] https://docs.python.org/3/library/functions.html#staticmethod

If it is a class method do not expect `self` as a first parameter, it is usually `cls` [1] Besides, I think that you whant `@staticmethod` here[2], in that case drop `self` [1] https://docs.python.org/3/library/functions.html#classmethod [2] https://docs.python.org/3/library/functions.html#staticmethod
Author
First-time contributor

That's obviously true for other methods I added, so blame me :)

That's obviously true for other methods I added, so blame me :)
Ghost marked this conversation as resolved
aplanas reviewed 2022-10-26 10:52:46 +02:00
lib/users.py Outdated
@ -0,0 +40,4 @@
@classmethod
def get_batch(self, db):
cur = db.cursor()
Owner

Use a with context, like in a file: https://www.psycopg.org/docs/cursor.html

Use a `with` context, like in a file: https://www.psycopg.org/docs/cursor.html
Author
First-time contributor

Should I refactor the code to use context managers everywhere?

Should I refactor the code to use context managers everywhere?
Author
First-time contributor

As you wish.

As you wish.
Owner

that would be awesome!

If you feel strong, refactor also the schema = dict() part, and build a list : ]

that would be awesome! If you feel strong, refactor also the `schema = dict()` part, and build a list : ]
Ghost marked this conversation as resolved
Ghost added 1 commit 2022-10-26 11:59:45 +02:00
Ghost requested review from Ghost Team 2022-10-26 11:59:58 +02:00
Ghost requested review from aplanas 2022-10-26 12:00:02 +02:00
Ghost approved these changes 2022-10-26 12:55:31 +02:00
Ghost left a comment
Author
First-time contributor

I guess we need to still define an email/username for fake accounts and unknown users. But this can be a 2nd step.

I guess we need to still define an email/username for fake accounts and unknown users. But this can be a 2nd step.
aplanas requested changes 2022-10-26 13:02:30 +02:00
lib/importer.py Outdated
@ -175,0 +160,4 @@
(lproject, lpackage) = row
self.update_db_package(db, lproject, lpackage)
missing_users = User.missing_users(db)
Owner

I think that from here you can remove all from the with context (at least I do not see any access to cur)

I think that from here you can remove all from the `with` context (at least I do not see any access to `cur`)
Ghost marked this conversation as resolved
Ghost added 1 commit 2022-10-26 13:46:53 +02:00
aplanas approved these changes 2022-10-26 13:47:54 +02:00
Ghost merged commit 87d9fcc131 into main 2022-10-26 14:05:48 +02:00
Ghost deleted branch user-db 2022-10-26 14:05:48 +02:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: importers/git-importer#3
No description provided.