Make --skip-existing less brittle

[mayeut]

twine checks for message starting with File already exists:

twine/twine/commands/upload.py

Lines 58 to 61 in bcb73e0

	# NOTE(sigmavirus24): Old PyPI returns the first message while Warehouse
	# returns the latter. This papers over the differences.
	msg = ('A file named "{0}" already exists for'.format(filename),
	'File already exists')

Message has been changed to one starting with The filename or contents already exist in pypi/warehouse#3310:
https://github.com/pypa/warehouse/blob/969695d030aef8b11d5b052e8731a80746c404c8/warehouse/forklift/legacy.py#L1083-L1094

c.f. failing deployment: https://travis-ci.org/mayeut/gcovr/builds/360424325

[brainwane]

Thanks for the report, @mayeut -- I suspect Twine should fix this, but thanks for reporting it in Warehouse too.

[di]

I fixed the regression in pypi/warehouse#3522, but we should modify this issue to something like "Make --skip-existing less brittle"

[theold190]

We have a similar problem in Artifactory 6.12.2 rev 61202900. Even if user uses --skip-existing, the twine fails with error.

Here is a message returned by Artifactory: "Not enough permissions to delete/overwrite artifact ..."
Here is a message expected by twine: "Not enough permissions to overwrite artifact"

So, current string doesn't match the one returned by Artifactory. Would be great to have a solution that would allow to avoid this kind of situation.

[bhrutledge]

@di I'm looking into this, thinking that an in check or regex might be sufficient. Do you have other suggestions?

I'm assuming that just checking the status code is insufficient.

twine/twine/commands/upload.py

Lines 24 to 48 in 5cb1867

	def skip_upload(response, skip_existing, package):
	filename = package.basefilename
	msg_400 = (
	# Old PyPI message:
	f'A file named "{filename}" already exists for',
	# Warehouse message:
	'File already exists',
	# Nexus Repository OSS message:
	'Repository does not allow updating assets',
	)
	msg_403 = 'Not enough permissions to overwrite artifact'
	# NOTE(sigmavirus24): PyPI presently returns a 400 status code with the
	# error message in the reason attribute. Other implementations return a
	# 409 or 403 status code. We only want to skip an upload if:
	# 1. The user has told us to skip existing packages (skip_existing is
	# True) AND
	# 2. a) The response status code is 409 OR
	# 2. b) The response status code is 400 AND it has a reason that matches
	# what we expect PyPI or Nexus OSS to return to us. OR
	# 2. c) The response status code is 403 AND the text matches what we
	# expect Artifactory to return to us.
	return (skip_existing and (response.status_code == 409 or
	(response.status_code == 400 and
	response.reason.startswith(msg_400)) or
	(response.status_code == 403 and msg_403 in response.text)))

[sigmavirus24]

@theold190 can you give us more details about the error? I'm disappointed that we're not getting a 429 from PyPI or Artifactory because that would be a reliable status code for a conflict (file exists and cannot be created)

[theold190]

@sigmavirus24 here is more information:

• issue was originally reproduced with twine==0.13.0, but it is the same behavior with twine==2.0.0.
• we are running next command: python -m twine upload --skip-existing --verbose --repository-url <REPOSITORY_URL> --repository <REPOSITORY_NAME> <FILE_TO_UPLOAD>
• issue is reproduced with Python 3 (Python 3.6.8)
• "--skip-existing" was working before we have done a minor update of Artifactory service 2-3 weeks ago
• content received from server:

Content received from server:
{
  "errors" : [ {
    "status" : 403,
    "message" : "Not enough permissions to delete/overwrite artifact '<ARTIFACT_INFORMATION>' (user '<USER>' needs DELETE permission)."
  } ]
}
HTTPError: 403 Client Error: Forbidden for url: <ARTIFACTORY_URL>

I am not sure what more information is needed.

My guess is that skip_upload code in twine/twine/commands/upload.py expects specific error message. And Artifactory returns a different one. Here are messages that I have already provided in previous comment (see difference marked with bold):

• "Not enough permissions to delete/overwrite artifact ..." (returned by Artifactory)
• "Not enough permissions to overwrite artifact" (expected by twine)

[sigmavirus24]

I just wanted the content received from the server. Sorry I wasn't specific enough. Thanks for all that detail.

@bhrutledge Perhaps we can remove the necessity to check the error message and instead provide what was returned on a log level such that users can debug things if we get an error code like this that could mean "Invalid credentials" just as easily as "You can't do that"?

[bhrutledge]

@sigmavirus24 Thanks for the suggestion. Can you elaborate on the resulting behavior of --skip-existing? A cursory scan of the code suggests to me that the 400 and 403 error codes are overloaded by PyPI and Artifactory, so a simple check like return skip_existing and status_code in [400, 403] would let the upload "succeed" with invalid credentials. It also seems like people rely on --skip-existing, so we wouldn't want to fail on any 400 or 403.

[theold190]

Thanks for a fast fix! We would like to get a new version of a package to test the fix. Does anyone know how to trigger a release process? Is it done on request or when there are enough fixes?

Information in https://github.com/pypa/twine/milestones seems to be a bit outdated.

[brainwane]

Could you make your own package of twine (from git master) to test locally? See https://packaging.python.org/tutorials/packaging-projects/ for how to package a project.

https://twine.readthedocs.io/en/latest/contributing.html#making-a-new-release steps 1, 2, 3, 4, 8, and 9 will help you run tests and make a twine package so you can install it into a local virtualenv to test.

Twine maintainers don't have a specific systematic "time to make a release" decision process as I understand it. As https://pypi.org/project/twine/#history shows, cadence has been more frequent recently. So I would guess it's likely there will be another point release in 2020.

[brainwane]

@theold190 The next release of Twine is out.

[theold190]

@brainwane, thanks for the heads up. I was able to verify that --skip-existing flag works as expected with locally build package and latest 3.0.0. version. Thanks for a fast fix and info!