Skip to content

chore: Data migration script for overlay edit_type field and corresponding pghistory event#979

Closed
adekoder wants to merge 2 commits intoNixOS:mainfrom
adekoder:data-migration-for-overlay-type-and-activity-log
Closed

chore: Data migration script for overlay edit_type field and corresponding pghistory event#979
adekoder wants to merge 2 commits intoNixOS:mainfrom
adekoder:data-migration-for-overlay-type-and-activity-log

Conversation

@adekoder
Copy link
Copy Markdown
Collaborator

@adekoder adekoder commented Apr 7, 2026

Description:
Migrate overlay edit_type values and event labels, Updates existing overlay records and pghistory event tables to use new field values:

Edit Type

  • add -> additional,
  • remove -> ignored

PGH Label

  • maintainers.add -> maintainer.add,
  • maintainers.remove -> maintainer.delete,
  • package.remove → package.ignore
  • package.add→ package.restore

Notes

  • Batched updates (1000 records)
  • Temporarily disables pghistory triggers on event tables, we need it disabled for the script to run which the script handles and also re-enabled the trigger once done.

ignored.click()
expect(active.get_by_text("alpha")).to_be_visible()
expect(active.get_by_text("charlie")).to_be_visible()
# Expand the ignored packages section before asserting since it will close due to re-render
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wasn't the previous version doing just that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it started failing, for some weird reason, and this fixed it


# let freeze the time for a bit long after the debounce time
# so that we delete event does not get folded to properly test
frozen_time.tick(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to add the new checks to the existing test. Maybe update the name. The setup takes quite long, also repeating test cases is very bad for readability.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is not a repeated testcase, this testcase it testing when there is a delete the activity log reflect that it was deleted based on the new event label conventions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But up to this line the test is identical to the previous one. Why not just continue that one?

return wrapped


@pytest.fixture()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes much sense moving that, since we're only using it in that one place

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

but fixtures should be in conftests

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah but not for those we use exactly once for a very particular test. I'm fine moving it when there's a need :)

@fricklerhandwerk
Copy link
Copy Markdown
Collaborator

The tests failed due to running out of space. We may need to increase the VM disk size:

defaults = {
documentation.enable = lib.mkDefault false;
virtualisation = {
memorySize = 2048;
cores = 2;
};

@adekoder
Copy link
Copy Markdown
Collaborator Author

adekoder commented Apr 8, 2026

The tests failed due to running out of space. We may need to increase the VM disk size:

defaults = {
documentation.enable = lib.mkDefault false;
virtualisation = {
memorySize = 2048;
cores = 2;
};

Ah, Thank you , i will fix it.

"is_event_table": True,
},
{
"class": MaintainerOverlayEvent,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I haven't looked at the PR in full yet, but I think this is wrong as it doesn't take into account edit _type which adds semantics for what it means to add/remove a maintainer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sorry, not sure i understand your comment, please clarify more ?

@fricklerhandwerk
Copy link
Copy Markdown
Collaborator

I guess we can close this as duplicate of #945

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.

3 participants