pagure-importer

#98 Some refactoring
Merged by cverna. Opened by pingou.
refactoring  into  master

Download 98.patch

This PR does a few things and each commit can be evaluated on its own, I tried to document the reasoning for the change in the commit messages, feel free to ask for more info if needed though.

In short what is happening:
- to_timestamp is moved out of the object since it does nothing with it, being a function instead of a method just makes sense then (I believe pylint might have pointed it out)
- Refactor the trac and pagure importer, the idea here is to allow for someone to get all the tickets of a trac/github project as JSON without having to deal with any of the git aspects. Basically, for migrating the rel-eng repo, I want to get the JSON blob of all the ticket in the releng trac on an existing, local git repo. Then I need to perform some checks and data manipulation, so I don't want to deal with this aspect of pagure-importer. So I moved the logic to clone the repo, generate the JSON blobs, commit and push to the CLI instead of it all being in the importer themselves. (Note: the commit is now done in a single step at the end which should also speed things up a little, especially for large repo)
- The last commit add a --nopush argument that allows to perform all the steps automatically except for pushing which can then be done manually, giving an opportunity to the user to review/inspect the changes before pushing them.

One question that I have been wondering is if the approach of inheritance with the Importer is really worth it.

Does it gain us much? I does make the code a little harder to re-use (especially outside of git repos, which is my primary idea here) and since click gives us two clearly different paths for trac vs github vs ... I am not sure inheritance brings us much here :)

I think we can get ride of the inheritance, I have added it to avoid code duplication but effectively we loose flexibility between github and fedorahosted.
So it might make sense to have both importer completely independent

I think we should still prompt the import progress.

i think we have to use from pagure_importer.utils import ....

I don't think this is needed .

needs to be self.clone_repo_location

same here

need to import issue_to_json

need to import issue_to_json

obj here is pagure_issue

Unless we drop the entire class :)

I originally starting my refactoring on an outdated clone, so that's likely a left over from an older time, +1 to remove

Either should work, but I also like best full-path imports, I was trying to be consistent with the import below that needs to go away anyway :)

okidoke

on both files indeed

project is also used here --> https://pagure.io/pagure-importer/blob/master/f/pagure_importer/utils/importer_trac.py#_102
So we might need to pass it as an argument of import_issues

Agreed²

it's passed onto the method as repo_name, so I replaced project by repo_name there

1 new commit added

  • Fix all the issues found during the review of the refactoring

need to replace newpath by folder here and line below

newpath --> folder

get_secure_filename needs to be imported from utils.git or move here but it is also used in importer_trac.py

These is reported as import not used.

Cleanup: import not used

1 new commit added

  • More import clean up and move get_secure_filename into the utils module

added is then used here --> https://pagure.io/pagure-importer/blob/master/f/pagure_importer/utils/git.py#_101
It is not needed anymore, so can be removed too

Some of the doc string missing ?

Also used here https://pagure.io/pagure-importer/blob/master/f/pagure_importer/utils/git.py#_107
Not sure if we need to keep that ?

https://pagure.io/pagure-importer/blob/master/f/pagure_importer/utils/git.py#_130 need to fix the commit message here obj is not defined

Just no finished :)

1 new commit added

  • Fix staging the changed files in update_git and provide the commit message as argument

missing a comma here

I have a working version in my fork refactoring branch (https://pagure.io/fork/cverna/pagure-importer/branch/refactoring).

I have removed the inheritance to the Importer class and cleaned up the git utils class.

I don't know what you want to do, I could push my changes to your branch or submit a PR from mine.

Regarding the performance, having only 1 commit at the end does not seems to gain us much time, although I did not do any proper profiling.
But I assume that what is greedy in time anyway are the JSON-RPC requests, we could look at sending requests in parallel.

Can you cherry-pick the commits that are just fixing this PR?

If so we could do it in a few steps:
a) finish this PR
b) open another PR for removing the inheritance and cleaning the code
c) look at making the requests in parallel

Sounds like we have a plan, I think I can cherry pick the commits to fix this PR.

5 new commits added

  • Cleaning importers code and passing nopush arg
  • Fix cloned repo location
  • Do no delete the cloned repo if nopush flag is true
  • Cleanup gitutils and remove not needed code
  • Fixing command to call fedorahosted and github import

@pingou Can you review my commits and then we can merge this PR .

1 new commit added

  • Removed unused variable and fix issue count logic for github import

Why not doing this outside of the with?

Nice one, do we know if this will work on all version of pygit2 we care about?

For my use-case I actually would like to specify the target folder to that method as I basically do not want to have to deal with the TracImporter object, or at least as little as possible. But maybe we can keep this for the next PR where we drop that object :)

when we exits the with this is called https://pagure.io/pagure-importer/blob/master/f/pagure_importer/utils/init.py#_255 and deletes the cloned repo.
So all operation on the git repos needs to be done in the with

hm, ok then no choice :)

pgimport requires pygit2 >= 0.22, this will work in that case :)
This was added in 0.20.3 https://github.com/libgit2/pygit2#0203-2014-04-02

Yeah will be better in a new PR :sweat_smile:

Cool, all clear to me :thumbsup:

Cool Thanks :)

Pull-Request has been merged by cverna
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.