augur icon indicating copy to clipboard operation
augur copied to clipboard

Fix: rename pr reviewers and labels

Open omkute10 opened this issue 1 month ago • 8 comments

Description

This PR refactors the database schema and related code to improve clarity and consistency in handling pull request reviewers and labels:

  1. Renamed PullRequestReviewer to PullRequestRequestedReviewer to better reflect its purpose
  2. Updated table name from pull_request_reviewers to pull_request_requested_reviewers
  3. Standardized column names in pull_request_labels to use pr_label_ prefix
  4. Updated all references throughout the codebase to use the new table and column names
  5. Added an Alembic migration to handle the schema changes

Testing

  • [x] Verified that the migration runs successfully
  • [x] Ensured all existing tests pass with the updated schema
  • [x] Test pull request creation with labels
  • [x] Verify pull request reviewers functionality

Notes for Reviewers

  • This is a breaking change that requires running the included migration
  • The changes are backward-incompatible due to table and column renames
  • All existing data will be preserved during migration
  • API consumers will need to update their queries to use the new table/column names

Signed commits

  • [x] Yes, I signed my commits.

omkute10 avatar Nov 06 '25 05:11 omkute10

Thanks for this fix!

I am planning to address various database and schema jank very soon, however I'm not quite there yet. These kinds of changes tend to be breaking changes as well.

I will likely be folding this change into some of the other changes I want to make, but I can't promise that will happen especially quickly. Im hoping for sometime in November but we shall see.

Im going to assign myself and mark this as pending changes as a reminder for me to return to this when i shift focus from the config system to the DB schema.

Thanks again for submitting this PR!

MoralCode avatar Nov 06 '25 19:11 MoralCode

This PR needs to pass CI. @MoralCode : Do you think this docker fail is related to the others we are seeing?

sgoggins avatar Nov 07 '25 15:11 sgoggins

the CI is failing with this exception:

 Traceback (most recent call last):
augur-1         |   File "/augur/.venv/bin/augur", line 10, in <module>
augur-1         |     sys.exit(run())
augur-1         |              ^^^^^
augur-1         |   File "/augur/.venv/lib/python3.11/site-packages/click/core.py", line 1462, in __call__
augur-1         |     return self.main(*args, **kwargs)
augur-1         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
augur-1         |   File "/augur/.venv/lib/python3.11/site-packages/click/core.py", line 1383, in main
augur-1         |     rv = self.invoke(ctx)
augur-1         |          ^^^^^^^^^^^^^^^^
augur-1         |   File "/augur/.venv/lib/python3.11/site-packages/click/core.py", line 1844, in invoke
augur-1         |     cmd_name, cmd, args = self.resolve_command(ctx, args)
augur-1         |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
augur-1         |   File "/augur/.venv/lib/python3.11/site-packages/click/core.py", line 1891, in resolve_command
augur-1         |     cmd = self.get_command(ctx, cmd_name)
augur-1         |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
augur-1         |   File "/augur/augur/application/cli/_multicommand.py", line 36, in get_command
augur-1         |     module = importlib.import_module('.' + name, 'augur.application.cli')
augur-1         |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
augur-1         |   File "/usr/local/lib/python3.11/importlib/__init__.py", line 126, in import_module
augur-1         |     return _bootstrap._gcd_import(name[level:], package, level)
augur-1         |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
augur-1         |   File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
augur-1         |   File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
augur-1         |   File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked
augur-1         |   File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
augur-1         |   File "<frozen importlib._bootstrap_external>", line 940, in exec_module
augur-1         |   File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
augur-1         |   File "/augur/augur/application/cli/db.py", line 27, in <module>
augur-1         |     from augur.application.db.models import Repo
augur-1         |   File "/augur/augur/application/db/models/__init__.py", line 1, in <module>
augur-1         |     from augur.application.db.models.augur_data import (
augur-1         | ImportError: cannot import name 'PullRequestReviewer' from 'augur.application.db.models.augur_data' (/augur/augur/application/db/models/augur_data.py)

Even if it passed CI though, i would like to take a deeper look at it as part of my database deep dive. Essentially im planning to adopt/take over this PR/incorporate it into a larger effort so that breaking changes all get made in one shot

MoralCode avatar Nov 07 '25 16:11 MoralCode

As an update, I think this PR is delayed by its sheer scope.

sgoggins avatar Dec 09 '25 21:12 sgoggins

@omkute10 : Do you plan to address the failing integration tests?

sgoggins avatar Dec 09 '25 21:12 sgoggins

yes, This PR is waiting for some other database changes to land first (notably #3435), as well as thinking through how the migration process will work for existing users (i hope postgres lets you RENAME a column, rather than creating a new one, copying data, and deleting the old one)

MoralCode avatar Dec 09 '25 22:12 MoralCode