-
Notifications
You must be signed in to change notification settings - Fork 190
Remove seed.sql to prevent installationId conflicts #1017
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
Conversation
…ents Co-Authored-By: noritaka.ikeda@route06.co.jp <noritaka.ikeda@route06.co.jp>
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Updates to Preview Branch (devin/1743075606-remove-seed-sql) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
NoritakaIkeda
left a comment
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.
LGTM! I'd love for others to take a look as well!
|
@junkisai cc @NoritakaIkeda It is certainly different from the installation ID of the Local GitHub App, so we should consider supporting it. |
junkisai
left a comment
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.
@MH4GF @NoritakaIkeda
I see, is my understanding correct as follows? 👀
seed.sqlis also applied to the Preview branch- Changing the GitHub App settings will cause the
installationIdto change
|
@junkisai
|
| @@ -1,19 +0,0 @@ | |||
| INSERT INTO "Project" | |||
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.
nits
If you plan to add similar data inserts again in the future, I’d recommend wrapping them in a BEGIN ... COMMIT transaction and using RETURNING to fetch the inserted IDs instead of hardcoding them.
That way, you can avoid relying on fixed IDs and ensure the data stays consistent even if something fails in the middle of the process.
Example for future reference:
BEGIN;
WITH inserted_project AS (
INSERT INTO "Project" (name, "updatedAt")
VALUES ('liam', now())
RETURNING id
),
inserted_repository AS (
INSERT INTO "Repository" (name, owner, "installationId", "updatedAt")
VALUES ('liam', 'liam-hq', 62899121, now())
RETURNING id
)
INSERT INTO "ProjectRepositoryMapping" ("projectId", "repositoryId", "updatedAt")
SELECT inserted_project.id, inserted_repository.id, now()
FROM inserted_project, inserted_repository;
INSERT INTO "WatchSchemaFilePattern" (pattern, "projectId", "updatedAt")
SELECT 'frontend/packages/db/prisma/**', id, now()
FROM inserted_project;
COMMIT;No need to change anything this time, just something to keep in mind when adding this kind of data setup again!
|
@junkisai I'll merge it for now — we can update the seed file later |
Issue
Why is this change needed?
This change removes the seed.sql file which contains a hardcoded GitHub App installationId. When the GitHub App is updated, this ID changes, causing issues in local development environments.
What would you like reviewers to focus on?
Confirm that removing this file is the correct approach to prevent local environment issues with GitHub App authentication.
Testing Verification
Verified that the file has been successfully removed.
Additional Notes
This PR only removes the seed.sql file as requested, with no other changes.
Link to Devin run: https://app.devin.ai/sessions/11ec8e3b59fd471ab71b71af7172309b
Requested by: noritaka.ikeda@route06.co.jp