-
Notifications
You must be signed in to change notification settings - Fork 297
feat: add command to clone project to local #4283
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
base: develop
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 18403653021Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
internal/clone/clone.go
Outdated
} | ||
// 3. Insert a row to `schema_migrations` | ||
fmt.Fprintln(os.Stderr, "Schema written to "+utils.Bold(path)) | ||
if shouldUpdate, err := utils.NewConsole().PromptYesNo(ctx, "Update remote migration history table?", true); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question
Why would a "pull" command "push" something up to the database ? This seems likely to cause error.
Also, what happen if the user "push" the migration, then continue to do changes on the dashboard and "clone" again ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to make it clear that clone
command is only used for initial setup. Subsequently, they should be using the specialised commands, ie. db pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have a hard time making sense of pushing migrations to the remote history table during clone
. 🤔
Is the prompt wrong?
As you say clone
is for going from an empty / non existing supabase
folder to an existing one directly fetched from a remote project right? So why would we even have existing migrations locally?
What am I missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why would we even have existing migrations locally?
The initial schema pulled from remote database would be the first migration isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but to me it doesn't make sense to push it during clone
. Clone is about dumping a remote project locally and should keep the remote untouched.
It would be pushed on first supabase db push
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The envisioned DX for users with an existing project would be
- supabase clone
- supabase start
Now he makes schema changes through local studio, then
- supabase db diff
- supabase db push
The problem is at step 4, we have 2 migrations: the cloned migration from 1 and the diffed migration from 3.
Historically we've been skipping the first migration by consulting the history table. That's why we ask users to initialise the history table on first pull.
But perhaps there's a better way to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or then we need to be more explicit about how we handle migrations and prompt the user something like:
It seems like you created changes in your database without using migrations. Would you like to turn these changes into a migration? <link to docs about migrations and why it's good for them>
Then with a global supabase push
we would push anything new in the local project (functions, migrations...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I'd really like clone to be a read only operation.
May be we can prompt whether to push the first migration (default behaviour right now) or simply mark it as applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to clone into schemas directory so users adopt declarative schemas by default.
Supabase start would use declared schemas if migrations are empty.
Then local studio changes are diffed into migrations directory. This way only incremental changes are pushed to remote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two cases here:
-
User just wants to spin up local dev for an existing remote project (changes live in dashboard).
→ In that case,clone
should stay read-only, never push anything. -
User wants to “eject” into code-based migrations.
→ We should make that transition explicit in the CLI, something like:You’ve cloned your project successfully Do you want to start managing migrations as code? • Make schema changes in local Studio → 1. run `supabase db diff > supabase/migrations/xxxx_migrations.sql` 2. Review the generated migration ensure it work locally with `supabase db reset` (will loose your local data) • Or write a migration manually → 1. `supabase migration new <name>` 3. Then apply locally → `supabase migration apply` And sync remote → `supabase db push`
That keeps clone
idempotent (like git clone
) while guiding users through the migration workflow.
We could also make db push
smarter:
if the first migration fails because the remote DB isn’t empty and has no supabase_migrations
schema, we can try/catch
that case, inspect the DB, and prompt:
“It looks like this isn’t a fresh database. Do you want to mark the migration as applied instead of running it? (In the case the migration contain changes you already performed on the remote database via dashboard / third party)”
That’d make the DX clearer and keep clone
safely read-only.
Edit: I know it's a bit vebose, but I'm leaning on the side of "explaining in the right context" rather than "put everyting in the docs" because at this point it might make more sense to the user. Also avoiding implicit behavior will increase the chances of having an user understanding what's done and how it works.
BTW, if possible, I would "print" the command we do for each migration every time.
(ex:
supabase db push
..Pushing migration to the database...
supabase db push -- supabase/migrations/XXXX_init.sql
It looks like this isn’t a fresh database. Do you want to mark the migration as applied instead of running it (y/n)? y
... Repairing migration mark it as passed on remote ...
supabase db repair -- supabase/migrations/XXXX_init.sql
..Pushing migration to the database...
supabase db push -- supabase/migrations/XXX_add_table.sql :: skipped
supabase db push -- supabase/migrations/XXX_add_table2.sql :: success
supabase db push -- supabase/migrations/XXX_add_table3.sql :: success
supabase db push -- supabase/migrations/XXX_add_table4.sql :: success
...
```)
I think even if more verbose, that'll allow user to more easily understand what's going on internally and also help them self-serve/debug when having troubles.
ece24dc
to
c6e059f
Compare
What kind of change does this PR introduce?
feature
What is the new behavior?
The vision here is that each command should follow the unix philosophy of building small single task programs that are composable. Unfortunately, this introduces a lot of fragmentation that affects the overall UX.
Hence, we are adding new commands to
quick start
section which is a composable stack of other sipmle commands.Additional context
Add any other context or screenshots.