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.

Sign up for GitHub

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

Jump to bottom

Trusted publishing: surface pending publisher name collisions #16260

Merged
merged 12 commits into from
Dec 2, 2024
Merged

Trusted publishing: surface pending publisher name collisions #16260

merged 12 commits into from
Dec 2, 2024

Conversation

twm
Copy link
Contributor

@twm twm commented Jul 11, 2024 •
edited
Loading

Provide up-front validation of the project name within a pending publisher by:

  • Extracting the validation logic from ProjectService.create_project to a new ProjectService.check_project_name routine
  • Update ProjectService.create_project to call check_project_name
  • Thread ProjectService into the forms in place of ProjectFactory.

Fixes #16226.

@di
Copy link
Member

di commented Jul 11, 2024

Hey @twm, FYI our general policy is to not review draft PRs unless explicitly asked -- let us know if you have any questions here, otherwise we will wait for this to become ready to review.

@twm twm force-pushed the 16226-oidc-name-validation branch from 595eed9 to 66a2c83 Compare July 13, 2024 05:08
@twm
Copy link
Contributor Author

twm commented Jul 13, 2024

Hi @di, thanks for approving the build! Some connectivity issues are making it difficult to get things working locally so it's very helpful. I'll remove the draft status when I'm ready for review!

@twm twm mentioned this pull request Jul 15, 2024
Closed
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from db324f3 to 2ab2a55 Compare July 17, 2024 05:08
twm
twm commented Jul 17, 2024
View reviewed changes
Copy link
Contributor Author

@twm twm left a comment

Choose a reason for hiding this comment

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

Hi @di, this is ready for review now!

warehouse/packaging/services.py Outdated Show resolved Hide resolved
warehouse/packaging/services.py Outdated Show resolved Hide resolved
@twm twm marked this pull request as ready for review July 17, 2024 18:45
@twm twm requested a review from a team as a code owner July 17, 2024 18:45
@twm twm force-pushed the 16226-oidc-name-validation branch from ed79c6b to 7033c15 Compare July 17, 2024 18:47
@twm twm mentioned this pull request Aug 2, 2024
Merged
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from 4065562 to 9cdb4ab Compare August 6, 2024 06:32
@twm twm force-pushed the 16226-oidc-name-validation branch 10 times, most recently from a8ede89 to 2182f86 Compare August 20, 2024 17:59
twm
twm commented Aug 20, 2024
View reviewed changes
warehouse/migrations/versions/f7720656a33c_index_ultranormalized_pending_project_.py
Comment on lines +51 to +52
CREATE INDEX pending_project_name_ultranormalized
ON pending_oidc_publishers (ultranormalize_name(project_name));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted against CREATE INDEX CONCURRENTLY here because the original migration didn't use it and I expect that there are orders of magnitude fewer pending_oidc_publishers rows than projects rows.

@twm twm force-pushed the 16226-oidc-name-validation branch from 2182f86 to 79daaed Compare August 20, 2024 18:13
@twm
Copy link
Contributor Author

twm commented Aug 20, 2024

Hi @di, this is ready again. I'd appreciate a review within a few days if you can, as I keep having to rebase through conflicts.

@twm twm force-pushed the 16226-oidc-name-validation branch from 79daaed to 0787590 Compare August 20, 2024 18:59
di
di reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

Just one last comment, otherwise LGTM!

warehouse/packaging/services.py Outdated Show resolved Hide resolved
@twm twm force-pushed the 16226-oidc-name-validation branch from 0787590 to c59f318 Compare September 21, 2024 22:51
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from 0c19353 to 13bd1d3 Compare September 22, 2024 18:27
@twm twm requested a review from di September 22, 2024 18:36
@twm
Copy link
Contributor Author

twm commented Sep 22, 2024

Hi @di, sorry for losing track of this. I've addressed your comment and merged forward.

di
di reviewed Sep 23, 2024
View reviewed changes
warehouse/packaging/services.py Outdated Show resolved Hide resolved
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from ac2cafd to 18bfa3f Compare September 24, 2024 21:38
@twm twm force-pushed the 16226-oidc-name-validation branch from 18bfa3f to 2902664 Compare September 24, 2024 21:45
@di di mentioned this pull request Nov 5, 2024
Closed
@di di enabled auto-merge (squash) November 25, 2024 15:53
auto-merge was automatically disabled December 2, 2024 02:59

Head branch was pushed to by a user without write access

@twm
Copy link
Contributor Author

twm commented Dec 2, 2024

Hi @di, thank you for the review! I've rebased the migration here again. The Dependencies build failure appears unrelated to these changes.

@di di enabled auto-merge (squash) December 2, 2024 17:47
@di di merged commit d6d91d5 into pypi:main Dec 2, 2024
20 checks passed
@di
Copy link
Member

di commented Dec 2, 2024

Thanks for your patience here @twm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@di di di approved these changes

Assignees
No one assigned
Labels
trusted-publishing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trusted publishing: pending publisher should warn about ultranormalized name collision
3 participants
@twm @di @miketheman