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

Add migration ci check #8867

Merged
merged 14 commits into from
Dec 9, 2024
Merged

Add migration ci check #8867

merged 14 commits into from
Dec 9, 2024

Conversation

Weiko
Copy link
Member

@Weiko Weiko commented Dec 4, 2024

Fixes #8865

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added a new CI check in the server workflow to detect uncommitted database migrations, ensuring schema changes are properly tracked with migration files.

  • Added migration check step in .github/workflows/ci-server.yaml that runs TypeORM's migration:generate for both core and metadata databases
  • Configured CI to fail if any migration files are generated, indicating missing schema change documentation
  • Added debug output capture for migration tool to assist in troubleshooting
  • Integrated with existing PostgreSQL service container for migration validation

1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

.github/workflows/ci-server.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-server.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-server.yaml Outdated Show resolved Hide resolved
await queryRunner.query(`ALTER TABLE "core"."workspaceSSOIdentityProvider" ALTER COLUMN "name" DROP NOT NULL`);
await queryRunner.query(`ALTER TABLE "core"."keyValuePair" ALTER COLUMN "textValueDeprecated" DROP NOT NULL`);
await queryRunner.query(`ALTER TABLE "core"."workspaceSSOIdentityProvider" ADD CONSTRAINT "CHK_SAML" CHECK ((((type = 'SAML'::core.idp_type_enum) AND ("ssoURL" IS NOT NULL) AND (certificate IS NOT NULL)) OR (type = 'OIDC'::core.idp_type_enum)))`);
await queryRunner.query(`ALTER TABLE "core"."workspaceSSOIdentityProvider" ADD CONSTRAINT "CHK_OIDC" CHECK ((((type = 'OIDC'::core.idp_type_enum) AND ("clientID" IS NOT NULL) AND ("clientSecret" IS NOT NULL)) OR (type = 'SAML'::core.idp_type_enum)))`);
Copy link
Contributor

Choose a reason for hiding this comment

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

To help me understand the migrations workflow, is it a line you add manually ?

);
await queryRunner.query(
`ALTER TABLE "core"."workspaceSSOIdentityProvider" ADD CONSTRAINT "FK_bc8d8855198de1fbc32fba8df93" FOREIGN KEY ("workspaceId") REFERENCES "core"."workspace"("id") ON DELETE CASCADE ON UPDATE NO ACTION`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

These two constraints are usefull and shouldn't be dropped:

await queryRunner.query(
      `ALTER TABLE "core"."workspaceSSOIdentityProvider" ADD CONSTRAINT "CHK_SAML" CHECK ((((type = 'SAML'::core.idp_type_enum) AND ("ssoURL" IS NOT NULL) AND (certificate IS NOT NULL)) OR (type = 'OIDC'::core.idp_type_enum)))`,
    );
    await queryRunner.query(
      `ALTER TABLE "core"."workspaceSSOIdentityProvider" ADD CONSTRAINT "CHK_OIDC" CHECK ((((type = 'OIDC'::core.idp_type_enum) AND ("clientID" IS NOT NULL) AND ("clientSecret" IS NOT NULL)) OR (type = 'SAML'::core.idp_type_enum)))`,
    );
    ```

Copy link
Contributor

Choose a reason for hiding this comment

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

Those ones too:

   await queryRunner.query(
      `ALTER TABLE "core"."appToken" ADD CONSTRAINT "userIdNotNullWhenTypeIsNotInvitation" CHECK (((type = 'INVITATION_TOKEN'::text) OR ("userId" IS NOT NULL)))`,
    );
    await queryRunner.query(
      `ALTER TABLE "core"."appToken" ADD CONSTRAINT "userIdIsNullWhenTypeIsInvitation" CHECK (((type <> 'INVITATION_TOKEN'::text) OR ("userId" IS NULL)))`,
    );

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion offline, let's remove those for now since typeorm does not handle Check annotation with expressions and take time to find a better implementation later

@FelixMalfait
Copy link
Member

Discussed with @AMoreaux in DM - he will had a few @Checks on the typeorm model that should simplify the migrations, then we can merge

@charlesBochet charlesBochet merged commit 23015de into main Dec 9, 2024
19 checks passed
@charlesBochet charlesBochet deleted the c--add-migration-ci-check branch December 9, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CI check to make sure typeorm models are always in sync with migrations
4 participants