Would it be appropriate to use query.one() here, or do you expect there to sometimes be more than one? What will handle the case when there are no results?

This is a lot of code to pile into an existing method. I strongly recommend putting all of the above code into a new function and calling that here instead.

You used new_key here without .strip(). One thing that might help is to set new_key = new_key.strip() at the top of the loop so that you don't have to remember to call .strip() everywhere.

This appears to be the same loop you performed above - why not put this code in that loop?

Perhaps it would help to add a validator to prevent these keys from having whitespace with a migration to remove the whitespace? Then you don't have to do this everywhere.

Does msg get used anywhere? If not, you might as well not save it.

This code block would be a good candidate to be its own method as well, to prevent this method from getting too long. This also makes testing easier, because the units are smaller and simpler.

One advantage of .first() is that it does not raise an error if there is no result, it simply returns None which I find easier to handle.

:thumbsdown: on adding queries to the model, queries are meant to be in lib.

@vivekanand1101 I added support for custom_keys, custom_fields, milestones and close_stats in the JSON of projects and issues in #1514.

Maybe you could rebase this branch on it (assuming it gets accepted) and then all we need is the part to load the data from the JSON.

Hm, what would raise this error?

So before we commit in the loop, and now we're doing it here, at the end, should we be consistent?

Couple of comments but that looks good.

This will require new unit-tests though (in addition to the existing ones)

Hm, what would raise this error?

Sorry, i missed this message and changed the code and now i can't find which error are you talking about here. @pingou

@bowlofeggs i changed a few things according to your suggestions. If you could look at this again, it would be nice.

I don't know why jenkins failed the last time. It is passing in mine.

Maybe just continue here, it should avoid the check below

Do we have example of keys not having a type specified?

Couple of questions but mostly nitpicking, so if you prefer we can merge as is

nope, as far as pagure-importer is concerned.

yup, doing it

So I'll propose we just do ['key_type'] as we have for names

So I'll propose we just do ['key_type'] as we have for names

Done. Actually, this part was written before the corresponding addition in pgimport. That's why i did that - to be safe.

Looks all good to me, thanks! :)