New column entry to separate source from project url #2568

mayaCostantini · 2022-01-31T15:51:27Z

Related Issues and Dependencies

Related to #2566

This introduces a breaking change

No

This should yield a new module release

Yes

This Pull Request implements

Create a label column separated from the url column in the python_package_metadata_project_url table to parse project URLs easily.

mayaCostantini · 2022-01-31T15:52:07Z

/assign @fridex

thoth/storages/graph/models.py

fridex

One comment. I wonder if we should handle old records in the migration. Otherwise 💯 👍🏻

fridex · 2022-01-31T16:17:09Z

thoth/storages/graph/models.py

@@ -1499,7 +1499,8 @@ class PythonPackageMetadataProjectUrl(Base, BaseExtension):
    __tablename__ = "python_package_metadata_project_url"

    id = Column(Integer, primary_key=True, autoincrement=True)
-    project_url = Column(Text, nullable=True)
+    url = Column(Text, nullable=True)
+    source = Column(Text, nullable=True)


It might be a good idea to name this as a label to follow the specification.

Thanks 💯 Changes were added in 7260f0b

thoth/storages/data/alembic/versions/2b787ddad4a4_separate_the_project_url_column_from_.py

mayaCostantini · 2022-02-02T15:25:03Z

/hold
Needs taking care of old records in the migration

mayaCostantini · 2022-02-03T21:49:27Z

/unhold

pacospace · 2022-02-04T09:20:14Z

/hold

pacospace · 2022-02-04T09:23:47Z

The approach is correct @mayaCostantini and very nice! There are some small things to adjust, I can send a PR to your branch for this to be included here :)

mayaCostantini · 2022-02-04T09:50:02Z

Thanks @pacospace , yes let's do it :)

pacospace · 2022-02-04T09:57:00Z

I opened mayaCostantini#1 to be merged in your branch and tested migration 🚀

mayaCostantini · 2022-02-04T11:24:21Z

@pacospace thanks for the changes 💯 , ready for review 👍

pacospace

One final instruction missing and we should be good to go :)

pacospace · 2022-02-04T11:54:41Z

thoth/storages/data/alembic/versions/2b787ddad4a4_separate_the_project_url_column_from_.py

+    )
+    op.bulk_insert(python_package_metadata_project_url_table, projects_source_url)
+    op.bulk_insert(has_metadata_project_url_table, metadata_project_url_links)
+    # ### end Alembic commands ###


I realized we need to add a last command: op.drop_column("has_metadata_project_url", "id") to be consistent with the rest of the database.

Ok, will do 👍

pacospace

Thanks! 💯

sesheta · 2022-02-04T12:59:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pacospace

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pacospace]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fridex · 2022-02-04T13:27:38Z

/hold

I think there could be one issue. Let me check it.

pacospace · 2022-02-04T13:43:38Z

/unhold

pacospace · 2022-02-04T13:44:00Z

/hold

pacospace · 2022-02-04T13:44:11Z

sorry I didn't see the comment

fridex

Finally had this on my list. PTAL on the comment.

fridex · 2022-02-07T18:36:48Z

thoth/storages/data/alembic/versions/2b787ddad4a4_separate_the_project_url_column_from_.py

+        {"python_package_metadata_id": r[0], "python_package_metadata_project_url_id": r[1]} for r in result_has
+    ]
+    op.drop_table("has_metadata_project_url")
+    op.drop_table("python_package_metadata_project_url")


I'm unsure here if dropping tables is correct. I think we will lose all the linkage between other tables. Could the query be rewritten so that tables are not dropped and python_package_metadata_project_url table is updated using:

UPDATE python_package_metadata_project_url SET label={label}, url={url} WHERE id={id}

I think that will simplify the migration and reduce any possible errors in the database.

goern · 2022-02-17T09:26:53Z

ping

mayaCostantini · 2022-02-17T10:16:56Z

@goern This needs testing using postgres which I can't do at the moment, I will come back to it as soon as my problem is solved

Signed-off-by: Francesco Murdaca <[email protected]>

mayaCostantini · 2022-03-18T17:25:00Z

/unhold
I could finally test my changes to the data migration and they seem to have worked. I just removed some entries where only the url was present but the label was missing, let me know if I should replace this value instead.

sesheta added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 31, 2022

sesheta requested review from harshad16 and KPostOffice January 31, 2022 15:51

sesheta assigned fridex Jan 31, 2022

pacospace reviewed Jan 31, 2022

View reviewed changes

thoth/storages/graph/models.py Show resolved Hide resolved

fridex reviewed Jan 31, 2022

View reviewed changes

sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 31, 2022

mayaCostantini force-pushed the correct-url-entries branch 3 times, most recently from bbf6eda to d3796f5 Compare February 2, 2022 12:41

pacospace reviewed Feb 2, 2022

View reviewed changes

thoth/storages/data/alembic/versions/2b787ddad4a4_separate_the_project_url_column_from_.py Outdated Show resolved Hide resolved

sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2022

mayaCostantini force-pushed the correct-url-entries branch from 841dfae to b82b8c9 Compare February 3, 2022 21:48

sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2022

sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2022

pacospace reviewed Feb 4, 2022

View reviewed changes

pacospace approved these changes Feb 4, 2022

View reviewed changes

sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2022

sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2022

sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2022

pacospace mentioned this pull request Feb 4, 2022

Adjust query for metadata due to change in schema #2579

Merged

fridex reviewed Feb 7, 2022

View reviewed changes

mayaCostantini and others added 8 commits March 16, 2022 16:42

New column entry to separate source from project url

1c7d19a

Alembic migration and rename source column to label

37881df

Remove instructions from migration

dbc9014

Handle data in the migration

44f9d05

Fix migration

510e536

Signed-off-by: Francesco Murdaca <[email protected]>

Pre-commit

720b8bd

Drop has_metadata_project_url column

69a61cf

Update python_package_metadata_project_url without dropping the table

b0a3823

mayaCostantini force-pushed the correct-url-entries branch from 174376c to b0a3823 Compare March 18, 2022 17:21

sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2022

sesheta merged commit f5dc180 into thoth-station:master Mar 18, 2022

mayaCostantini deleted the correct-url-entries branch June 16, 2022 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New column entry to separate source from project url #2568

New column entry to separate source from project url #2568

mayaCostantini commented Jan 31, 2022 •

edited

Loading

mayaCostantini commented Jan 31, 2022

fridex left a comment

fridex Jan 31, 2022

mayaCostantini Jan 31, 2022

mayaCostantini commented Feb 2, 2022

mayaCostantini commented Feb 3, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022 •

edited

Loading

mayaCostantini commented Feb 4, 2022

pacospace commented Feb 4, 2022

mayaCostantini commented Feb 4, 2022

pacospace left a comment

pacospace Feb 4, 2022

mayaCostantini Feb 4, 2022

pacospace left a comment

sesheta commented Feb 4, 2022

fridex commented Feb 4, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022

fridex left a comment

fridex Feb 7, 2022

goern commented Feb 17, 2022

mayaCostantini commented Feb 17, 2022

mayaCostantini commented Mar 18, 2022

New column entry to separate source from project url #2568

New column entry to separate source from project url #2568

Conversation

mayaCostantini commented Jan 31, 2022 • edited Loading

Related Issues and Dependencies

This introduces a breaking change

This should yield a new module release

This Pull Request implements

mayaCostantini commented Jan 31, 2022

fridex left a comment

Choose a reason for hiding this comment

fridex Jan 31, 2022

Choose a reason for hiding this comment

mayaCostantini Jan 31, 2022

Choose a reason for hiding this comment

mayaCostantini commented Feb 2, 2022

mayaCostantini commented Feb 3, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022 • edited Loading

mayaCostantini commented Feb 4, 2022

pacospace commented Feb 4, 2022

mayaCostantini commented Feb 4, 2022

pacospace left a comment

Choose a reason for hiding this comment

pacospace Feb 4, 2022

Choose a reason for hiding this comment

mayaCostantini Feb 4, 2022

Choose a reason for hiding this comment

pacospace left a comment

Choose a reason for hiding this comment

sesheta commented Feb 4, 2022

fridex commented Feb 4, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022

pacospace commented Feb 4, 2022

fridex left a comment

Choose a reason for hiding this comment

fridex Feb 7, 2022

Choose a reason for hiding this comment

goern commented Feb 17, 2022

mayaCostantini commented Feb 17, 2022

mayaCostantini commented Mar 18, 2022

mayaCostantini commented Jan 31, 2022 •

edited

Loading

pacospace commented Feb 4, 2022 •

edited

Loading