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

Refactor and consolidate CSV network importers #579

Open
mrchrisadams opened this issue Apr 25, 2024 · 0 comments
Open

Refactor and consolidate CSV network importers #579

mrchrisadams opened this issue Apr 25, 2024 · 0 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed importers ops / infra Not user facing, but aimed making the application more operable

Comments

@mrchrisadams
Copy link
Member

mrchrisadams commented Apr 25, 2024

We currently have two kinds of CSV importers in our codebase for importing IP addresses for a provider - one was a external contribution a few years back.

The two importers

There is the new style CSVImporter importer that follows the ImporterProtocol, so the importing logic is shared with the other ones that import the protocol.

This is in apps.greenchcheck.importers.importers_csv.CSVImporter, and is used in our admin UI.

There is also the older ImporterCSV class that was created before, and contains its own network importer logic, and only supports IP4 imports.

This is in apps.greencheck.bulk_importers.ImporterCSV

This is used in our management command, import_from_csv.

What I think we should do

This is a bit confusing, and really we should only have one.

Really, we ought to be only using the newer version, and removing the ImporterCSV, so all our CSV importing uses the same code path, and we have consistent signatures for creating instances of the class.

If there are any helpful exceptions to raise and display to end users, this would be the time to make those changes too, probably.

@mrchrisadams mrchrisadams added help wanted Extra attention is needed good first issue Good for newcomers ops / infra Not user facing, but aimed making the application more operable importers labels Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed importers ops / infra Not user facing, but aimed making the application more operable
Projects
None yet
Development

No branches or pull requests

1 participant