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

[sqlserver] Ensure version table in provided schema #840

Merged
merged 2 commits into from Dec 31, 2023

Conversation

alex-kuck
Copy link
Contributor

Closes #839

@coveralls
Copy link

coveralls commented Oct 26, 2022

Coverage Status

coverage: 59.226% (+0.02%) from 59.21%
when pulling 0350a00 on alex-kuck:fix/839/sqlserver_version_table
into 0815e2d on golang-migrate:master.

@alex-kuck alex-kuck force-pushed the fix/839/sqlserver_version_table branch from c89e846 to 9da9d3c Compare October 26, 2022 11:53
@alex-kuck
Copy link
Contributor Author

@dhui @Fontinalis Do you agree with this solution, or would you prefer to always create the version table in schema dbo instead?

@germanozambelli
Copy link

Is there any news? I too need this PR to be merged

@zhenriquegomes
Copy link

Any news about this PR?

@alex-kuck alex-kuck force-pushed the fix/839/sqlserver_version_table branch from 515114c to 7823a01 Compare November 29, 2023 14:06
@alex-kuck
Copy link
Contributor Author

Rebased and fixed linter issues, so should be good to go 🤞

@alex-kuck
Copy link
Contributor Author

@dhui Sorry to bother you personally, but is there anything I/we can do to get this merged? Thanks! 🙏

@alex-kuck alex-kuck force-pushed the fix/839/sqlserver_version_table branch from 8a6ff0c to 0350a00 Compare December 25, 2023 09:29
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

@alex-kuck Thanks for the PR and sorry for the delayed response!
Overall, I'm onboard with having the schema migration version table stored with the custom schema since this allows multiple projects/apps to use golang-migrate with a single sqlserver.
Is this change backwards compatible? e.g. what happens with existing db instances that use a custom schema? I believe that the "old" migration version table would be stored in dbo but after this change, golang-migrate would now expect it to be in the custom schema.

@alex-kuck
Copy link
Contributor Author

Hi @dhui, thanks for your review and no worries, I'm just happy to get this one out the door 😄
I don't think this would be an issue. I believe in the current implementation - i.e. pre-PR - the schema dbo is only explicitly used for existence check of the migration table. All other access - including the actual creation - uses the default schema of the connection (i.e. without explicit schema specification). So I think there are basically three cases:

  1. User has no default schema and no schema is set in config -> SQL Server default dbo is used everywhere. So migration table was created there and will still be used from there after the PR
  2. User has default schema dbo and/or config schema is explicitly set to dbo -> same as 1.
  3. User has different default schema xyz and/or different schema is set in config -> migration table is still in xyz but error described in issue will no longer occur, since existence check uses xyz

Hope this makes sense 😅

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

@alex-kuck Thanks again for the PR and your patience!
Also thanks for answering my questions and enumerating the various scenarios. It looks like custom schemas weren't supported by the sqlserver db driver before this PR. It looks like this was a bug with the initial implementation where the dbo schema was explicitly specified in the migration table existence check but not any other usage.

@dhui dhui merged commit 516f038 into golang-migrate:master Dec 31, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sqlserver] ensureVersionTable() errors with custom schema
5 participants