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

DEV: First pass at process_uploads script #26662

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

s3lase
Copy link
Contributor

@s3lase s3lase commented Apr 17, 2024

The process_upload script is meant to be a refactor of the existing uploads importer script at /script/bulk_import/uploads_importer.rb.

This change mostly duplicates the existing implementation with minor adjustments as a first step.

@github-actions github-actions bot added the migrations-tooling PR which includes changes to migrations tooling label Apr 17, 2024
The `process_upload` script is meant to be a refactor of the existing uploads importer
script at `/script/bulk_import/uploads_importer.rb`.

This change mostly duplicates the existing implementation with minor adjustments as a first step.
This change mostly splits up various parts of the existing implementation
with a lot of duplications. The idea is more to test out a potential structure for
making these tasks granular and composable.
@s3lase s3lase force-pushed the dev/refactor-uploads-importer branch from 990f463 to b5c89fb Compare April 22, 2024 11:12
configure_zeitwerk("lib/common")

module Uploads
class CLI < Thor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to keep CLI code in bin/process_uploads. It's not really reusable code that you'd expect to find in lib. It doesn't help with testability, either. But maybe I'm missing something, what are your arguments for putting it into lib?

Copy link
Contributor Author

@s3lase s3lase Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be great for if we ever want to compose the CLIs into a single "migration" CLI, making each a sub command will be relatively straightforward.

It doesn't help with testability, either.

Could you explain this further?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrations-tooling PR which includes changes to migrations tooling
2 participants