Skip to content

Comments

Add D1 mode to migrate.sh#5

Open
hirefrank wants to merge 1 commit intomainfrom
codex/audit-codebase-for-efficiency-improvements
Open

Add D1 mode to migrate.sh#5
hirefrank wants to merge 1 commit intomainfrom
codex/audit-codebase-for-efficiency-improvements

Conversation

@hirefrank
Copy link
Owner

Summary

  • combine D1 migration logic into main script using --d1
  • drop migrate_d1.sh
  • document using the --d1 flag in README

Testing

  • bash -n migrate.sh

https://chatgpt.com/codex/tasks/task_e_6842d9d663d0833082aaaece89705fac

@arimendelow
Copy link

I'll say I tried using ChatGPT when writing my fork of your script and I think it really struggles with making coherent bash scripts 😆

For background, the reason I forked your script instead of making a PR is that I think this functionality is different enough that it makes generalizing the logic far too complex for a scripting language. Too many edge cases. A proper typed language could probably handle all these logic forks, but I find complex bash to be nearly alien.

Some initial notes:

  • There's a whole host of logic that got duplicated that could be shared, eg the sed that does the conversion and the pg_dump
  • Codex took out a bunch of important comments, eg around the foreign key commands and the weirdness with what's required where for D1. I did actually open a bug on wrangler for that, here: Wrangler D1 import — succeeds as expected locally but fails on remote with useless error cloudflare/workers-sdk#9503
  • The original script is broadly useful, but I designed the fork to only be useful for RedwoodSDK — an example of that is that Codex held on to the migration commands, which don't make sense in this context.

In general, rather than merging the logic, it seems that Codex basically copied in the forked script alongside the existing one, which sort of defeats the purpose. I appreciate the thought here wrt making One Script to Rule Them All, though I think it's really okay for this niche use case to live in its own script.

For broader context, my intention is to eventually create a toolkit for migrating from https://github.com/redwoodjs/graphql to https://github.com/redwoodjs/sdk — the script is only part of that.

That said, if you want to forge ahead with this, let me know! I'd be happy to help you come up with a robust set of test cases.

@arimendelow
Copy link

OH @hirefrank — the one simple change that you can certainly incorporate to your script is the one for converting the date format — https://gist.github.com/arimendelow/e60f8a1303dc00bd4a21e84142623dbc#file-psql-to-d1-sh-L67

Note that you'll need to switch from just doing sed to doing sed -E to support this regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants