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

Make migrations work for us in water-abstraction-system #106

Closed
Cruikshanks opened this issue Nov 16, 2023 · 1 comment
Closed

Make migrations work for us in water-abstraction-system #106

Cruikshanks opened this issue Nov 16, 2023 · 1 comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project

Comments

@Cruikshanks
Copy link
Member

water-abstraction-system is already using the migration engine we want to use (Knex migrations). But we are working in this 'weird-world' where the migrations we've written are focused on creating tables that already exist. This is because we are dealing with a legacy service with an existing DB.

Because of this, we can only run these migrations against a test DB. We are now working on a feature where for the first time we need to create our tables. We'd love to do this in water-abstraction-system but because of how we've set things up have managed to lock ourselves out of doing this. We can't just add our new 'proper' migrations amongst the ones we've created to support unit tests.

Fortunately, there does appear to be a way out. Currently, our knexfile.js configures the migration config inside the default config.

const defaultConfig = {
  client: 'postgres',
  useNullAsDefault: true,
  migrations: {
    tableName: 'knex_migrations',
    directory: './db/migrations'
  },
  seeds: {
    directory: './db/seeds'
  }
}

We then refer to this config when generating the config for each environment (development, test, production).

const development = {
  ...defaultConfig,
  connection: defaultConnection
}

const test = {
  ...defaultConfig,
  connection: {
    ...defaultConnection,
    database: DatabaseConfig.testDatabase
  }
}

const production = {
  ...defaultConfig,
  connection: defaultConnection
}

So, Knex already gives us the ability to do something different for each environment. Reading the docs it tells us that migrations: { directory: '' } can be an array. We can even tell Knex to treat the migrations in each directory separately with sortDirsSeparately:.

So, for development and production we can just run our 'proper' migrations. In test we should be able to configure it to run the current migrations that recreate legacy tables, and then our 'proper' migrations.

@Cruikshanks Cruikshanks added the housekeeping Refactoring, tidying up or other work which supports the project label Nov 16, 2023
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 20, 2023
DEFRA/water-abstraction-team#106

This project is already using the migration engine we want to use ([Knex migrations](https://knexjs.org/guide/migrations.html)). But we are working in a 'weird-world' where the migrations we've written are focused on creating tables that already exist. This is because we are dealing with a legacy service with an existing DB.

Because of this, we can only run these migrations against a test DB. We are now working on a feature where for the first time we need to create 'real' tables. We'd _love_ to do this in **water-abstraction-system** but because of how we've set things up have we've managed to lock ourselves out of doing this. We can't just add our new 'proper' migrations amongst the ones we've created to support unit tests.

This change is about sorting that problem. Now when we run migrations in a non-test environment only those found in `db/migrations/public` will be run. However, in our CI and when running unit tests locally we can run the existing create legacy table migrations followed by the 'real' migrations.

To confirm this we add 'real' migrations to create 3 new views. Going forward when we need to reference a legacy table we intend to do so through a view. Doing so means we can

- rename the 'table' to reflect what is actually in it!
- rename fields that annoy us, for example, `foo_bar_wiggle_thompson_id` just becomes `id` 😁
- rename fields to be standard, for example, `date_created` becomes `created_at`
- ignore fields that don't seem to be populated, or that don't vary

We also tidy up the commands we use in `package.json` as we need to make changes to support this.
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 20, 2023
DEFRA/water-abstraction-team#106

This project is already using the migration engine we want to use ([Knex migrations](https://knexjs.org/guide/migrations.html)). But we are working in a 'weird-world' where the migrations we've written are focused on creating tables that already exist. This is because we deal with a legacy service with an existing DB.

Because of this, we can only run these migrations against a test DB. We are now working on a feature where for the first time we need to create 'real' tables. We'd _love_ to do this in **water-abstraction-system** but we've managed to lock ourselves out of doing this because of how we've set things up. We can't just add our new 'proper' migrations amongst the ones we've created to support unit tests.

This change is about sorting that problem. Now when we run migrations in a non-test environment only those found in `db/migrations/public` will be run. However, in our CI and when running unit tests locally we can run the existing create legacy table migrations followed by the 'real' migrations.

To confirm this we add 'real' migrations to create 3 new views. Going forward when we need to reference a legacy table we intend to do so through a view. Doing so means we can

- rename the 'table' to reflect what is actually in it!
- rename fields that annoy us, for example, `foo_bar_wiggle_thompson_id` becomes `id` 😁
- rename fields to be standard, for example, `date_created` becomes `created_at`
- ignore fields that don't seem to be populated, or that don't vary

We also tidy up the commands we use in `package.json` as we need to make changes to support this.
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 20, 2023
DEFRA/water-abstraction-team#106

To deploy this using our existing scripts we have to be able to call `npm run migrate`. If we kept the naming convention we would have to rewrite our deployment scripts, so it is easier if we just re-name our scripts in here.
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 20, 2023
DEFRA/water-abstraction-team#106

To deploy this using our existing scripts we have to be able to call `npm run migrate`. If we kept the naming convention we would have to rewrite our deployment scripts, so it is easier if we just rename our scripts in here.
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 20, 2023
DEFRA/water-abstraction-team#106

As part of the work we have been doing to get migrations working in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system) we are going to be creating all our new tables in a single schema `public`.

We have also decided that when there is a legacy table that we are still going to need for SROC. We are going to create a View of that table in the `public` schema and correct any issues with naming conventions, unused fields etc in the view. The first of these views that represent the old `returns` tables have been created in #531

This PR will create the models, helpers and unit tests required to be able to start to use these new views.
Jozzey added a commit to DEFRA/water-abstraction-service that referenced this issue Nov 22, 2023
DEFRA/water-abstraction-team#106

As part of making the migrations work in `water-abstraction-system` we have been creating DB Views of the legacy Returns data that better represent the data as we use it in our new nervice. Part of this exercise is pruning out the unused columns that exist in the legacy tables that are not utilised.

However we have found that for some of the columns in the legacy data, whilst pointless, still need to be populated as they are set to Not Nullable in the legacy database tables. We are therefore going to set a default value for these columns in the legacy DB so that they are always populated even when they do not exist in our new Views.

The tables that will have migrations written for them in this PR are:
- `returns.regime` - this is always "water" so will default to "water"
- `returns.licence_type` - this is always "abstraction" so will default to "abstraction"
- `lines.substance` - surprise, surprise, this is always "water" so will default to "water"
- `lines.unit` - This is always "m³" so will default to "m³"
Jozzey added a commit to DEFRA/water-abstraction-returns that referenced this issue Nov 22, 2023
DEFRA/water-abstraction-team#106

As part of making the migrations work in `water-abstraction-system` we have been creating DB Views of the legacy Returns data that better represent the data as we use it in our new service. Part of this exercise is pruning out the unused columns that exist in the legacy tables that are not utilised.

However, we have found that for some of the columns in the legacy data, whilst pointless, still need to be populated as they are set to Not Nullable in the legacy database tables. Therefore, we will set a default value for these columns in the legacy DB so that they are always populated even when they do not exist in our new Views.

The table columns that will be affected by this PR are:
- `returns.regime` - this is always "water" so it will default to "water"
- `returns.licence_type` - this is always "abstraction" so it will default to "abstraction"
- `lines.substance` - surprise, surprise, this is always "water" so it will default to "water"
- `lines.unit` - This is always "m³" so it will default to "m³"
Jozzey added a commit to DEFRA/water-abstraction-returns that referenced this issue Nov 23, 2023
DEFRA/water-abstraction-team#106

As part of making the migrations work in `water-abstraction-system` we have been creating DB Views of the legacy Returns data that better represent the data as we use it in our new service. Part of this exercise is pruning out the unused columns that exist in the legacy tables that are not utilised.

However, we have found that for some of the columns in the legacy data, whilst pointless, still need to be populated as they are set to Not Nullable in the legacy database tables. Therefore, we will set a default value for these columns in the legacy DB so that they are always populated even when they do not exist in our new Views.

The table columns that will be affected by this PR are:
- `returns.regime` - this is always "water" so it will default to "water"
- `returns.licence_type` - this is always "abstraction" so it will default to "abstraction"
- `lines.substance` - surprise, surprise, this is always "water" so it will default to "water"
- `lines.unit` - This is always "m³" so it will default to "m³"
Jozzey added a commit to DEFRA/water-abstraction-system that referenced this issue Nov 23, 2023
DEFRA/water-abstraction-team#106

As part of the work we have been doing to get migrations working in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system) we are going to be creating all our new tables in a single schema `public`.

We have also decided that when there is a legacy table that we are still going to need for SROC. We are going to create a View of that table in the `public` schema and correct any issues with naming conventions, unused fields etc in the view. The first of these views that represent the old `returns` tables have been created in #531

This PR will create the models, helpers and unit tests required to be able to start to use these new views.
@Jozzey
Copy link
Contributor

Jozzey commented Nov 23, 2023

@Jozzey Jozzey closed this as completed Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

No branches or pull requests

2 participants