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

Revert "Merge pull request #2311 from UKGovernmentBEIS/3005-new-dsit-… #2333

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

CristinaRO
Copy link
Collaborator

…org-id"

Changes in this PR

This reverts commit 8ba7b20, reversing changes made to 49e892f.

This migration doesn't fail, but the database changes (which happen before the new code is deployed and serving requests) cause an interval of incompatibility with the old code (which is still serving requests).

The right way to do this would be a data migration or rake task.

For now the plan is to roll back this code so it doesn't cause server errors on production, and then either:

  • deploy a workaround to support both transparency IDs in parallel
  • use a data migration, if we are granted access within a reasonable timeframe for the project

Screenshots of UI changes

N/A

Next steps

  • Is an ADR required? An ADR should be added if this PR introduces a change to the architecture.
  • Is a changelog entry required? An entry should always be made in CHANGELOG.md, unless this PR is a small tweak which has no impact outside the development team.
  • Do any environment variables need amending or adding?
  • Have any changes to the XML been checked with the IATI validator? See XML Validation

…org-id"

This reverts commit 8ba7b20, reversing
changes made to 49e892f.
@CristinaRO CristinaRO requested review from mec and shuldt January 22, 2024 15:56
@CristinaRO CristinaRO mentioned this pull request Jan 22, 2024
@CristinaRO
Copy link
Collaborator Author

Copy link
Collaborator

@mec mec left a comment

Choose a reason for hiding this comment

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

When we delete this schema migration, we'll leave a 'hole' in the migration list - are we happy to have that? Otherwise we need to rollback the migratiion before reverting this commit?

I also cannot see the changes reverting for 49e892f - am I missing something?

@CristinaRO
Copy link
Collaborator Author

CristinaRO commented Jan 23, 2024

When we delete this schema migration, we'll leave a 'hole' in the migration list - are we happy to have that? Otherwise we need to rollback the migratiion before reverting this commit?

It doesn't really "leave a hole" - staging and dev have already ran it, and it's "just" data, not structure. The way it was written means:

  • if we re-add the migration with the same timestamp, staging and dev will not try to re-run it - there is a record in the metatable schema_migrations with that timestamp, so staging and dev "know" they've already run a migration with this timestamp
  • if we re-add the migration with a different timestamp, or re-write in a different way, we will have to write it in a way that doesn't cause errors; this is true, and as long as we know it, we can handle it; this is what it's doing by checking that service_owner isn't nil, so it doesn't try to execute actions on a nil object.

Training and production won't know anything about it, the timestamp has never reached their meta-table, so they'll just run whatever comes their way as normal.

I also cannot see the changes reverting for 49e892f - am I missing something?

We don't need to revert the dependency merge. We're only pulling out the merges (that's why I'm reverting the merge commits, not the individual commits) that contained migrations.

@CristinaRO CristinaRO merged commit 83a7c9f into develop Jan 23, 2024
3 checks passed
@CristinaRO CristinaRO deleted the revert-service-owner-migration branch January 23, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants