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

fix(data-migrate): provide option to use db in dist instead of src #8375

Merged
merged 14 commits into from
May 24, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented May 19, 2023

Note This is not breaking; merely additive.

Fixes #8059. This PR:

  1. separates the data migrate commands (up and install) into yargs-metadata and handler files (just a chore step to align with the rest of the CLI commands)
  2. makes it so that yarn rw data-migrate up can run using the lib/db in dist instead of src:
yarn rw data-migrate up --import-strategy "dist"

Right now, yarn rw data-migrate up imports db from api/src/lib. I think the logic is, we use the require hook anyway to run the migration scripts, so we may as well use it to import the db from api/src/lib too. But we now realize that api/src may not always be available.

@razzeee I want to confirm this solves your use case in case you're set up is different than I thought. Now api/src/lib/db no longer needs to be available, but api/dist/lib/db does.

@jtoar jtoar added the release:fix This PR is a fix label May 19, 2023
@replay-io
Copy link

replay-io bot commented May 19, 2023

18 replays were recorded for 02659dc.

image 0 Failed
image 18 Passed

View test run on Replay ↗︎

@jtoar jtoar marked this pull request as ready for review May 19, 2023 23:16
@razzeee
Copy link
Contributor

razzeee commented May 20, 2023

This sounds like a step forward, I think.

I'm wondering, if it would improve things, if we would be able to pass a path, like prisma does to the schema. But I'm unsure. (It probably only saves you from changing folders when using a global install of rw)

@jtoar
Copy link
Contributor Author

jtoar commented May 22, 2023

@razzeee would the path be to the dataMigrations directory?

@razzeee
Copy link
Contributor

razzeee commented May 22, 2023

I think it should be the path to the dist? unless it would work with the dataMigration folder only. if I could pass that path, I would assume, I would be able to only copy that folder to a machine and be able to run stuff, which probably won't work?

@jtoar
Copy link
Contributor Author

jtoar commented May 22, 2023

@razzeee the rw CLI already supports the --cwd flag and RWJS_CWD env var; maybe what you want to do already works via one of them?

rw --cwd path/to/project data-migrate up
# or
RWJS_CWD=path/to/project rw data-migrate up

Just in case I still don't understand, here's what data-migrate up expects:

  • api/dist (which is just used as a check to make sure api is built)
  • api/dist/lib/db.js (to get the Prisma client)
  • api/dataMigrations (has the data migrations to run)

I could still think about a --dist-path flag though if that's better, which would be the path to api/dist.

@razzeee
Copy link
Contributor

razzeee commented May 22, 2023

Yeah, I think --dist-path is what I would be looking for in that situation. For e.g. if I decided to name that folder something that is not dist or want to call it from a different path than I'm on right now.

@jtoar
Copy link
Contributor Author

jtoar commented May 23, 2023

Ok @razzeee I think this is gtg but let me run it by a few others first

@jtoar jtoar added the fixture-ok Override the test project fixture check label May 23, 2023
@jtoar jtoar added this to the next-release-patch milestone May 24, 2023
@jtoar jtoar merged commit a7334cf into main May 24, 2023
4 checks passed
@jtoar jtoar deleted the ds-cli/fix-data-migrations branch May 24, 2023 03:14
jtoar added a commit that referenced this pull request May 24, 2023
…8375)

* separate out up, install handlers

* remove needless async

* add missing @

* fix import

* use values not entries

* fix data structure

* add back early return

* rename to camel case

* feat: support `--dist-path` flag

* follow up fixes

* apply suggestions from review
@jtoar jtoar modified the milestones: next-release-patch, v5.2.3 May 24, 2023
jtoar added a commit that referenced this pull request May 24, 2023
…8375)

* separate out up, install handlers

* remove needless async

* add missing @

* fix import

* use values not entries

* fix data structure

* add back early return

* rename to camel case

* feat: support `--dist-path` flag

* follow up fixes

* apply suggestions from review
@jtoar
Copy link
Contributor Author

jtoar commented May 24, 2023

@razzeee I just released this PR in an RC: 5.2.3-rc.3+ddbd1f431. You can upgrade to it via yarn rw upgrade -t rc. The hash after the + may not be there, but it shouldn't matter. Would love to know if this fixes it for you

@jtoar jtoar modified the milestones: v5.2.3, next-release May 24, 2023
@razzeee
Copy link
Contributor

razzeee commented May 25, 2023

@jtoar I tried to validate this, but I seem to run into something unrelated. I can't access the deployed frontend anymore, after deploying with my (partly) reverted changes. Haven't really looked at db migrations due to that.

Anyway, spend enough time on this for this week, I can probably have another go next week.

Copy link
Contributor Author

jtoar commented May 25, 2023

No worries I appreciate you trying!

@jtoar jtoar modified the milestones: next-release, next-release-patch, v5.2.4 Jun 2, 2023
jtoar added a commit that referenced this pull request Jun 2, 2023
…8375)

* separate out up, install handlers

* remove needless async

* add missing @

* fix import

* use values not entries

* fix data structure

* add back early return

* rename to camel case

* feat: support `--dist-path` flag

* follow up fixes

* apply suggestions from review
@jtoar jtoar modified the milestones: v5.2.4, next-release, next-release-patch Jun 2, 2023
jtoar added a commit that referenced this pull request Jun 2, 2023
…8375)

* separate out up, install handlers

* remove needless async

* add missing @

* fix import

* use values not entries

* fix data structure

* add back early return

* rename to camel case

* feat: support `--dist-path` flag

* follow up fixes

* apply suggestions from review
@jtoar
Copy link
Contributor Author

jtoar commented Jun 2, 2023

@razzeee this just landed in v5.2.4, so no need to upgrade to an RC to try it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: rw data-migration looks for src folder
2 participants