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

Enqueue CSVs from the CLI #322

Merged
merged 2 commits into from
Feb 11, 2021
Merged

Enqueue CSVs from the CLI #322

merged 2 commits into from
Feb 11, 2021

Conversation

adrianna-chang-shopify
Copy link
Contributor

This should be the last missing piece for CSV functionality in the gem. This PR allows a file to be passed via a --csv option on the CLI in order to run CSV processing Tasks from the command line.

Changes

  • 039e3e9 adds a --csv option to the CLI. If a file is supplied, we build an attachable (see https://edgeguides.rubyonrails.org/active_storage_overview.html#attaching-file-io-objects) to send to Runner.run. The CLI will surface the error if the file doesn't exist / isn't openable / etc.
  • ab1ee8c modifies Runner.run to verify that the Task processes CSVs before attaching the CSV (since a user can now pass a CSV for a non-CSV Task via the CLI).
    • Why not do this check in the CLI? This would involve performing Task.named from the CLI to check if the Task includes the CsvCollection module, and I felt like the CLI should not be aware of such constants and the details of how Task and CsvCollection are related
  • 84074ee this commit could potentially be dropped - it makes it so that if a CSV task is run from the Runner without a csv_file supplied, we raise.
    • Why wouldn't we want this? The system will fail gracefully without this code, in the same way that it does if you run a CSV Task from the UI without attaching a file:
      Screen Shot 2021-02-04 at 12 15 16 PM
    • Why would we want this? It offers a better UX from the CLI in my opinion, as we fail early without enqueuing and then they know to supply a CSV from the command line.
  • 8867b51 updates the docs, including adding documentation for using Runner.run to run a CSV Task (which is possible today, but not documented)

Tophatting

The following scenarios are relevant to 🎩
In test/dummy:

  • be maintenance_tasks perform 'Maintenance::ImportPostsTask' =>
    • produces error Maintenance::ImportPostsTask must be supplied with a CSV file to process.
    • If we drop 84074ee, will enqueue successfully, and error Cannot parse nil as CSV will be surfaced in the UI
  • be maintenance_tasks perform 'Maintenance::ImportPostsTask' --csv 'invalid.csv' =>
    • produces error No such file or directory @ rb_sysopen - invalid.csv
  • be maintenance_tasks perform 'Maintenance::ImportPostsTask' --csv '../fixtures/files/sample.csv'
    • Make sure to use inline queue adapter / Sidekiq / etc.. so this runs
    • This should be successful
  • be maintenance_tasks perform 'Maintenance::UpdatePostsTask' --csv '../fixtures/files/sample.csv'`
    • Should be successful
    • Should not attach a CSV to the Run for this Task

@adrianna-chang-shopify adrianna-chang-shopify added the CSV Requirements for CSV feature label Feb 4, 2021
Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

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

We could also support --csv - for stdin, and also just loading from the stdin if it's not a TTY when using pipes like curl https://example.com/posts.csv | bundle exec maintenance_tasks perform Maintenance::ImportPostsTask but than can be done in another PR.

lib/maintenance_tasks/cli.rb Outdated Show resolved Hide resolved
lib/maintenance_tasks/cli.rb Outdated Show resolved Hide resolved
app/models/maintenance_tasks/runner.rb Outdated Show resolved Hide resolved
app/models/maintenance_tasks/runner.rb Outdated Show resolved Hide resolved
app/models/maintenance_tasks/run.rb Outdated Show resolved Hide resolved
app/models/maintenance_tasks/run.rb Outdated Show resolved Hide resolved
app/models/maintenance_tasks/run.rb Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSV Requirements for CSV feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants