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.

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

Improve pypi package expected releasers pre-check. #5669

Merged
merged 3 commits into from Apr 10, 2018

Conversation

Projects
None yet
4 participants
@kwlzn
Copy link
Member

kwlzn commented Apr 5, 2018

Post-release fixup. Hacked around this for the 1.6.0rc0 release in order to not complicate the commit stream.

@kwlzn kwlzn requested review from stuhood , jsirois and wisechengyi Apr 5, 2018

@@ -101,7 +102,7 @@ def check_ownership(users):
users = set(user.lower() for user in users)
non_expected_users = users.difference(expected_package_owners)
if non_expected_users:
raise ValueError('{} are not expected releasers. You may want to get added, as per https://www.pantsbuild.org/release.html#owners and then add yourself to expected_package_owners in file build-support/bin/release.py'.format(", ".join(non_expected_users)))
raise ValueError('{} are not expected releasers. You may want to get added, as per https://www.pantsbuild.org/release.html#owners and then add yourself to expected_package_owners in file src/python/pants/releases/packages.py'.format(", ".join(non_expected_users)))

This comment has been minimized.

@stuhood

stuhood Apr 5, 2018

Member

Hm. Given where it runs, this check seems more annoying than helpful.

This comment has been minimized.

@kwlzn

kwlzn Apr 5, 2018

Member

I thought so too. happy to eliminate the check altogether if you think that makes more sense.

This comment has been minimized.

@stuhood

stuhood Apr 5, 2018

Member

From looking at it, it's not clear why there is a literal set, given that https://github.com/twitter/pants/blob/18da8cfdd5e715f5b4deeecb046f7284fab4e061/src/python/pants/releases/packages.py#L109 checks ownership using actual live values.

@stuhood

stuhood approved these changes Apr 5, 2018

@kwlzn kwlzn changed the title Add kwlzn to expected package owners. Eliminate expected releasers hardcoded pre-check in favor of live checking ownership mappings. Apr 5, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Apr 9, 2018

I added this to cover the case of:
If you are the first person to publish a package, you'll be the only owner, and the next person to do a release will not be able to do so.

I'm happy with making the check less aggressive, or happen at a different time, but I think it's a useful check to keep to avoid single-owner packages...

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Apr 9, 2018

@illicitonion afaict, live ownership checking against the authenticated pypi user still happens per-package, just sans whitelist in this case. so I don't think this extra top level check is buying anything fwict - except for a need to explicitly manage a whitelist.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Apr 10, 2018

This was specifically a response to #5628 where I performed a release, and ended up being the only owner of a package. This was only detected when @mateor tried to perform the next release.

Which check are you saying exists which would catch this before someone tries to perform a release and finds themselves blocked on the previous releaser?

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Apr 10, 2018

none - but in terms of release dry-run coverage I'm pretty willing to trade off the management of an explicit whitelist of pypi usernames in source control for "the next releaser sees this before any meaningful releasing takes place", which is good enough I think?

but: how about a happy medium heuristic of len(owners) > 3 instead?

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Apr 10, 2018

Sure, I can live with len(owners) > 3 :)

@kwlzn kwlzn force-pushed the twitter:kwlzn/release_auth branch from 4ce3271 to dfc65f1 Apr 10, 2018

@kwlzn kwlzn changed the title Eliminate expected releasers hardcoded pre-check in favor of live checking ownership mappings. Improve pypi package expected releasers pre-check. Apr 10, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

@kwlzn kwlzn merged commit 0f904aa into pantsbuild:master Apr 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment