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

Adding support for schema management in snowflake #402

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

abhinavcohesity
Copy link
Contributor

@abhinavcohesity abhinavcohesity commented Jun 2, 2020

I had to close the earlier pull request as I had by mistake pushed 2 commits.

However I have addressed the comments in that pull request.
Link for the earlier pull request.

  • Made gofmt changes so that "golangci-lint" doesn't give any errors.
  • Added "build_snowflake.go" in internal/cli.
  • Also since Snowflake is a hosted service, there are no dockerfiles for testing locally. Hence I havent
    added any tests. However I have tested manually against my private snowflake hosted instance.

@coveralls
Copy link

coveralls commented Jun 2, 2020

Pull Request Test Coverage Report for Build 780

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 53.38%

Totals Coverage Status
Change from base Build 775: 0.0%
Covered Lines: 2614
Relevant Lines: 4897

💛 - Coveralls

@abhinavcohesity
Copy link
Contributor Author

@dhui Any more comments on this one ? I had closed the earlier pull request as I had by mistake pushed 2 commits in that one.

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.

Did you run go mod tidy?

|------------|---------------------|-------------|
| `x-migrations-table` | `MigrationsTable` | Name of the migrations table |

Snowflake is PostgreSQL compatible but has some specific features (or lack thereof) that require slightly different behavior.
Copy link
Member

Choose a reason for hiding this comment

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

Add a note about Snowflake not being officially supported since there aren't any tests.

If you'd like, you can also link to this driver in top-level README, but you don't have to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a line saying that Snowflake cant be made to run locally.

@@ -0,0 +1,373 @@
// +build go1.12
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@abhinavcohesity
Copy link
Contributor Author

Did you run go mod tidy?

Ran it.

@abhinavcohesity
Copy link
Contributor Author

Build is failing with go1.14 due to some issues with mssql/server testing.

@abhinavcohesity
Copy link
Contributor Author

@dhui any more changes needed ? Or else we can merge ?

@dhui dhui merged commit 79396ff into golang-migrate:master Jun 4, 2020
@Uzair614
Copy link

@dhui Possible to create another tag? Last one was 4.11.0 in April which doesn't contain these changes.

@dhui
Copy link
Member

dhui commented Jul 27, 2020

Released v4.12.0 with these changes

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.

None yet

4 participants