-
-
Notifications
You must be signed in to change notification settings - Fork 739
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
chore: add migration for release plans #8529
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
CREATE INDEX IF NOT EXISTS idx_milestones_release_plan_definition_id ON milestones (release_plan_definition_id); | ||
|
||
ALTER TABLE IF EXISTS release_plan_definitions | ||
ADD COLUMN IF NOT EXISTS active_milestone_id SERIAL REFERENCES milestones(id); |
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.
Certainly this is an INT (you don't want a separate sequence generated 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.
Yep 100% correct
|
||
CREATE TABLE IF NOT EXISTS milestone_strategies | ||
( | ||
id SERIAL PRIMARY KEY NOT NULL, |
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.
Why not a textual id here as well?
CREATE TABLE IF NOT EXISTS milestone_strategies | ||
( | ||
id SERIAL PRIMARY KEY NOT NULL, | ||
milestone_id SERIAL NOT NULL REFERENCES milestones(id) ON DELETE CASCADE, |
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.
Same thing here, the type of the FK side of the relationship isn't another SERIAL, it's an INT.
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.
Though, I'd suggest maybe having all these new tables have textual PKs
src/migrations/20241024062521-add-release-plans-milestones-strategies.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Christopher Kolstad <[email protected]>
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, we're making a cyclic dependency between release_plan_definitions and milestones?
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.
Nice. LGTM
Addressed in chat - But for clarity: yes. It's intentional but shouldn't be cyclic. a release_plan_definition will be added, milestones will be added to this release plan, then we can set an active milestone via the release_plan_definition.active_milestone_id. |
Adds release plans migration.
Adds a down migration that removes indexes, columns, and tables in an order that addresses inter-dependencies