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

Remove sqlite support #3084

Merged
merged 4 commits into from
Sep 6, 2022
Merged

Remove sqlite support #3084

merged 4 commits into from
Sep 6, 2022

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Aug 31, 2022

Summary

This PR removes sqlite support from both the server and tests. The server --dbFile flag, and config file options will no longer do anything. From now on a postgreSQL database is required to run the server.

In tests, if the POSTGRESQL_CONNECTION environment variable is set, the tests will run using that postgres instance. If it's not set in CI the tests will fail. If it's not set outside of CI (local development) any tests that require the database will be skipped.

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

Great to see this step getting to the finish line

case !ok && isEnvironmentCI:
t.Fatal("CI must test all drivers, set POSTGRESQL_CONNECTION")
case !ok:
t.Skip("Set POSTGRESQL_CONNECTION to test against postgresql")
Copy link
Collaborator

Choose a reason for hiding this comment

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

So many of our tests rely on this I wonder if we should always fail without it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests for our main interfaces (CLI, API, config) do all rely on a database, but there are plenty of tests for internal things that do not. It's easy to change, but I think it can be nice to let people run tests with minimal setup.

@@ -646,6 +647,8 @@ type testCaseLabel struct {
Line string
}

var isEnvironmentCI = os.Getenv("CI") != ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this variable live closer to there it is used? I dont see it in migrations_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used 6 lines below here. The diff looks weird because I removed it from a different place in the data package.

This removes sqlite support.
Previously it was in a different place because we wanted different behaviour for the data package
vs other packages. Now that we don't support sqlite we can use the same behaviour everywhere.
@dnephin dnephin force-pushed the dnephin/remove-sqlite-support branch from b61e19d to 29d1dd0 Compare September 6, 2022 15:57
@dnephin
Copy link
Contributor Author

dnephin commented Sep 6, 2022

I pushed one more commit that removed a couple things I had missed:

  • removes data.NewSQLiteDriver
  • one test in models was using that call, so it was updated to use database.PostgresDriver
  • removed sqlite testing of migrator

Those changes allow us to remove our dependency on gorm.io/driver/sqlite and mattn/go-sqlite3.

@dnephin dnephin merged commit 0f5cade into main Sep 6, 2022
@dnephin dnephin deleted the dnephin/remove-sqlite-support branch September 6, 2022 16:54
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.

2 participants