Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update the check_schema_delta script to account for when the schema version has been bumped locally #15466

Merged
merged 5 commits into from
Apr 25, 2023
Merged

Update the check_schema_delta script to account for when the schema version has been bumped locally #15466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aac5f5c
add a check to check_schema_delta to account for when the schema vers…
H-Shay Apr 20, 2023
f5c59dd
newsfragment
H-Shay Apr 20, 2023
bb21437
fix commit number
H-Shay Apr 20, 2023
13dae79
check that the nw version is allowable before using as canonical version
H-Shay Apr 21, 2023
165fec5
read from local file to see if schema has changed
H-Shay Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15466.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update the check_schema_delta script to account for when the schema version has been bumped locally.
26 changes: 24 additions & 2 deletions scripts-dev/check_schema_delta.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

To summarise, we previously enforced (and continue to enforce) the invariant

PRs can only introduce migrations for the current schema

but we have now altered "current schema" to mean the PR's current schema version, falling back to develop's schema version.

I think we ought to have a check which enforces that the PR's schema version is either (develop's schema version) or (develop's schema version + 1). That would prevent the following footgun:

  • Develop is on schema version 1.
  • Someone opens PR X which bumps the schema version from 1 to 2. We ignore it for ages.
  • Develop sees several schema updates. It's now on schema 5.
  • We rerun the CI for PR X which still targets version 2.
  • It passes.
  • ⚠️ We merge in new migrations to schema delta 2, after it should have been frozen!

Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,32 @@ def main(force_colors: bool) -> None:
exec(r, locals)
current_schema_version = locals["SCHEMA_VERSION"]

click.secho(f"Current schema version: {current_schema_version}")

diffs: List[git.Diff] = repo.remote().refs.develop.commit.diff(None)

# Get the schema version of the local file to check against current schema on develop
with open("synapse/storage/schema/__init__.py", "r") as file:
local_schema = file.read()
new_locals: Dict[str, Any] = {}
exec(local_schema, new_locals)
local_schema_version = new_locals["SCHEMA_VERSION"]

if local_schema_version != current_schema_version:
# local schema version must be +/-1 the current schema version on develop
if abs(local_schema_version - current_schema_version) != 1:
click.secho(
"The proposed schema version has diverged more than one version from develop, please fix!",
fg="red",
bold=True,
color=force_colors,
)
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of looping through the diff we can just read the local file in the checkout? Which will be the merge commit?

click.get_current_context().exit(1)

# right, we've changed the schema version within the allowable tolerance so
# let's now use the local version as the canonical version
current_schema_version = local_schema_version

click.secho(f"Current schema version: {current_schema_version}")

seen_deltas = False
bad_files = []
for diff in diffs:
Expand Down