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

Migrate driver adapters to prisma/prisma #4380

Merged
merged 30 commits into from
Oct 27, 2023
Merged

Conversation

miguelff
Copy link
Contributor

@miguelff miguelff commented Oct 24, 2023

Sibling to prisma/prisma#21601 (merge that PR first, then this engines' one)

This PR:

  • Assumes driver adapters code reside in prisma/prisma
  • Defines convenient make tasks for testing the query engine against driver adapters, which does the following:
    • Ensure you've got a working copy of prisma/prisma in the filesystem at the same level than prisma-engines
    • Symlink driver adapter packages within query-engine/driver-adapters
    • Build the driver adapters and the connector kit to run queries through the driver adapters
    • Run tests using the connector kit for running queries
    • All this complexity is hidden behind make tasks. To run query-engine test through driver adapters:
      • DRIVER_ADAPTER=$adapter make test-qe where $adapter is one of pg, neon, libsql or planetscale

To ease working on changes to both driver adapters and rust code (if a feature or bugfix spans both codebases), we jus work normally locally, being prisma/prisma checked out in the same directory of prisma/prisma-engines. When running the make task above, the symlinks will be updated and hence the new code of driver adapters will be used.

And then to test in CI, we denote which branch of prisma/prisma do we want to use for tests, by applying a convention on the commit message. As such, if we are working on branch fix-planetscale-adapter in prisma/prisma and want to query engine tests to use the driver adapters code in that branch, we can include the DRIVER_ADAPTERS_BRANCH=fix-planetscale-adapter in the message of the commit we want to test. By default, driver adapters tests will use the main version of prisma/prisma driver adapters. a merge commit on prisma-engines will use main in prisma/prisma, so the order of merging PRs that span both codebases is:

  • First merge changes in prisma/prisma
  • Then merge changes in prisma-engines

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 24, 2023

CodSpeed Performance Report

Merging #4380 will not alter performance

Comparing driver-adapters-migration (7eb2831) with main (3305ecc)

Summary

✅ 11 untouched benchmarks

@miguelff miguelff self-assigned this Oct 24, 2023
@Jolg42 Jolg42 added this to the 5.6.0 milestone Oct 25, 2023
@miguelff miguelff force-pushed the driver-adapters-migration branch from 6476541 to 03913ca Compare October 25, 2023 11:11
@miguelff miguelff force-pushed the driver-adapters-migration branch from 460a44d to 9d37568 Compare October 25, 2023 11:30
@miguelff miguelff force-pushed the driver-adapters-migration branch 2 times, most recently from 2f28960 to 73e4e2d Compare October 25, 2023 12:07
@miguelff miguelff force-pushed the driver-adapters-migration branch from 73e4e2d to 418c50d Compare October 25, 2023 12:54
@miguelff miguelff force-pushed the driver-adapters-migration branch from ee06dbe to 96d4cd0 Compare October 25, 2023 13:44
Copy link
Contributor Author

@miguelff miguelff left a comment

Choose a reason for hiding this comment

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

Commented on the more relevant changes in this PR

- name: Extract Branch Name
id: extract-branch
run: |
branch="$(git show -s --format=%s | grep -o "DRIVER_ADAPTERS_BRANCH=[^ ]*" | cut -f2 -d=)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to test the query engine code against a particular prisma/prisma branch in CI. So we are not depending on merge ordering of prisma/prisma changes.

Example:

  • Feature requires changes in the engine and driver-adapters
  • changes in driver adapters are in branch foo of prisma/prisma
  • If we want to test a particular commit in the engines against those changes we just need to push a commit in here containing DRIVER_ADAPTERS_BRANCH=foo.

@@ -80,6 +87,10 @@ dev-sqlite:
dev-libsql-sqlite: build-qe-napi build-connector-kit-js
cp $(CONFIG_PATH)/libsql-sqlite $(CONFIG_FILE)

test-libsql-sqlite: dev-libsql-sqlite test-qe-st

test-driver-adapter-libsql: test-libsql-sqlite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

convention over configuration to run driver adapters tests easily. See README.md changes.

build-connector-kit-js: build-driver-adapters symlink-driver-adapters
cd query-engine/driver-adapters/connector-test-kit-executor && pnpm i && pnpm build

build-driver-adapters: ensure-prisma-present
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We ensure prisma/prisma is our FS, and build driver adapters from source.

@cd ../prisma && pnpm --filter "*adapter*" i && pnpm --filter "*adapter*" build
@echo "Driver adapters build completed.";

symlink-driver-adapters: ensure-prisma-present
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Then we symlink driver adapters code

@ln -sfn "../prisma/tsconfig.build.adapter.json" "./tsconfig.build.adapter.json"; \
echo "Symbolic links creation completed.";

ensure-prisma-present:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. If locally we have a clone of prisma, we are done, if that clone diverges from main we will warn the user. If we don´t have a clone (f.i. in CI) we shallowly clone it -- takes less than 10 seconds

Comment on lines +1 to +3
node_modules
adapter-*
driver-adapter-utils
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignoring symlinks from prisma/prisma

Comment on lines +23 to +27
"@prisma/adapter-libsql": "../adapter-libsql",
"@prisma/adapter-neon": "../adapter-neon",
"@prisma/adapter-pg": "../adapter-pg",
"@prisma/adapter-planetscale": "../adapter-planetscale",
"@prisma/driver-adapter-utils": "../driver-adapter-utils",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on symlinked driver adapters

@miguelff miguelff marked this pull request as ready for review October 25, 2023 14:13
@miguelff miguelff requested a review from a team as a code owner October 25, 2023 14:13
@miguelff miguelff requested review from jkomyno and Weakky and removed request for a team October 25, 2023 14:13
@Jolg42
Copy link
Contributor

Jolg42 commented Oct 26, 2023

(Still reviewing)

@miguelff can you remove this workflow? https://github.com/prisma/prisma-engines/blob/main/.github/workflows/publish-driver-adapters.yml

Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

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

Tested and reviewed together with @miguelff, with a few minor improvements approved together + @Jolg42's suggestion to remove a deprecated workflow file.

@janpio
Copy link
Contributor

janpio commented Oct 26, 2023

Did a full review, generally looks good but 2 remarks:

  1. Stating the obvious that tests are failing with:
git clone --depth=1 https://github.com/prisma/prisma.git --branch= ../prisma
Cloning into '../prisma'...
warning: Could not find remote branch  to clone.
fatal: Remote branch  not found in upstream origin
make: *** [Makefile:308: ensure-prisma-present] Error 128
Error: Process completed with exit code 2.
  1. Per README this is supposed to check out main of prisma/prisma. But does that not mean that prisma-engines tests are not not deterministic any more, as a change to prisma/prisma can make the same commit here pass or fail?

@miguelff
Copy link
Contributor Author

miguelff commented Oct 27, 2023

  1. Stating the obvious that tests are failing with

Fixed

  1. Per README this is supposed to check out main of prisma/prisma. But does that not mean that prisma-engines tests are not not deterministic any more, as a change to prisma/prisma can make the same commit here pass or fail?

That is true, but I don't consider very negative. Let me explain why:

  • While pushing commits to an engines PR, we would understand if something fails, as soon as something buggy was merged in prisma, instead of waiting for any integration PR (that was also the case, but reversed from prisma -> prisma/engines) when the driver adapters where in here.

  • To the best of my knowledge, the only edge case where the non-determinism can play against us, would be this sceanario:

    1. a PR in engines sitting inactive, while some changes in main where merged in prisma/prisma.
    2. then engines PR is merged
    3. merged commit is tested against newer main of prisma, which was never tested before, and fails.

    I think this can happen but should be odd enough. To happen it means:

    • that no changes: in prisma/prisma-engines main happened in the mean time, or they were ignored and merge of the engines PR was done before updating the branch first. Because otherwise, a new update commit will be added to the PR, thus exercising the new main.
    • prisma/prisma tests did not provide enough fidelity, i.e. to merge prisma/main integration tests should have passed. If that was the case, then it would not very expected to make integration tests for prisma-engines fail, If so, we would be signaled by a lack of coverage in prisma/prisma, and informed to act on it.

All in all, this is not a perfect solution, but we can try as an intermediate step towards monorepo, that would allow us to develop faster across the two codebases (more or less as fast for engines developers) and faster for prisma/prisma developers which will have test feeeback instantly after modifying any driver adapter code.

@miguelff miguelff merged commit 6dda9d7 into main Oct 27, 2023
49 of 55 checks passed
@miguelff miguelff deleted the driver-adapters-migration branch October 27, 2023 11:51
miguelff added a commit that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: driver adapters formerly phase 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants