-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add scope to redirects #315
Conversation
@@ -19,5 +19,7 @@ export class Migration20220721122758 extends Migration { | |||
'alter table "PageTreeNode" add constraint "PageTreeNode_parentId_foreign" foreign key ("parentId") references "PageTreeNode" ("id") on update cascade on delete set null;', | |||
); | |||
this.addSql('create index "PageTreeNode_parentId_index" on "PageTreeNode" ("parentId");'); | |||
|
|||
this.addSql('alter table "Redirect" add column "scope_domain" text 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.
I don't think it is good practice to alter existing migrations. If the starter would be deployed, this change would not be made.
Create a new migration and also make it fail-safe by truncating the table before.
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.
#315 (comment)
Please discuss this with @johnnyomair and let me know what I should do ^^
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'm sorry, this was my suggestion 😅 We did this multiple times in the past to avoid having a bunch of migrations when starting a new project. We could change our approach though
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.
@dkarnutsch what do you prefer?
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.
@nsams what do you think? Maybe we should start adding actually working migrations once the Starter has been deployed
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.
A growing Number of Migrations in New Projects Is Not desirable, that is unnecessarily confusing.
Maybe we can drop database and run fixtures on every starter deploy?
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.
@dkarnutsch what do you think?
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.
Maybe we can drop database and run fixtures on every starter deploy?
I'm okay with that (out of scope for this PR)
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'll take care of this (COM-1106)
Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>
5ef4e0d
PR Checklist