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

db-migrations: use pg for new migrations #1363

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Jan 14, 2025

  • move existing migrations to lib/model/migrations/legacy to dissuade creating new knex-based migrations
  • implement replacement for existing knex migrator
  • add eslint warnings for app-level imports into migrations files

TODO

What has been done to verify that this works as intended?

CI

Why is this the best possible solution? Were any other approaches considered?

Could use slonik instead, but seems better to tie implementation to a lower-level library, especially given concerns about upgrading to a more recent slonik (there are large API changes) and/or possibility of moving to an alternative to slonik.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Intended effects: none
Possible effects: break production databases

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Shouldn't require docs changes.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

} catch (err) {
console.error('Error:', err.message);
console.error(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert this change - it was just for debugging.

@@ -28,10 +28,10 @@ NODE_CONFIG_DIR=../../config DEBUG=knex:query,knex:bindings npx knex migrate:up
*/

const config = require('config');
const { connectionObject } = require('../util/db');
const { knexConfig } = require('../util/db');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create a separate PR for this.

@@ -0,0 +1,23 @@
// Copyright 2025 ODK Central Developers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is included for demo purposes - it will actually be included by #1356

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.

1 participant