-
Notifications
You must be signed in to change notification settings - Fork 80
[PULP-946] Implement PEP 658 #1031
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
base: main
Are you sure you want to change the base?
Conversation
848fa89 to
31cd665
Compare
5e4f2cb to
69e1326
Compare
gerrod3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good.
| remote=self.remote, | ||
| deferred_download=self.deferred_download, | ||
| ) | ||
| d_artifacts.append(metadata_artifact) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing we need to do is add the metadata_sha256 to the package since the PyPI json api doesn't have the value.
package.metadata_sha256 = md_sha256.
| finally: | ||
| if temp_wheel_path and os.path.exists(temp_wheel_path): | ||
| os.unlink(temp_wheel_path) | ||
| if temp_metadata_path and os.path.exists(temp_metadata_path): | ||
| os.unlink(temp_metadata_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice to ensure we clean up the files, but if we assume this method is always called in either a task or with a tmp_dir do we need to clean up the files? The tmp_dir should autocleanup, even on error, deleting the tmp files with it, so I think we can remove this.
| html_content = response.text | ||
| assert f'data-dist-info-metadata="sha256={PYTHON_WHEEL_METADATA_SHA256}' in html_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add this check into ensure_simple. Pass in a new metadata_sha_digests dict that should check if certain links have this field set correctly.
| """ | ||
| Test that the sync of a Python wheel package creates a metadata artifact. | ||
| """ | ||
| remote = python_remote_factory(includes=PYTHON_XS_PROJECT_SPECIFIER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should choose a different package since this test runs in parallel meaning the upload tests will also be creating the same content. All the wheels on the fixtures should have the metadata file available.
| created_count = 0 | ||
| skipped_count = 0 | ||
|
|
||
| with tempfile.TemporaryDirectory() as temp_dir: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to want to use settings.WORKING_DIRECTORY here.
| packages = ( | ||
| PythonPackageContent.objects.filter(metadata_sha256__isnull=False) | ||
| .exclude(metadata_sha256="") | ||
| .prefetch_related("contentartifact_set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .prefetch_related("contentartifact_set") | |
| .prefetch_related("contentartifact_set") | |
| .only("filename", "metadata_sha256") |
| metadata_artifact.save() | ||
| except IntegrityError: | ||
| metadata_artifact = artifact_model.objects.get( | ||
| sha256=metadata_artifact.sha256, pulp_domain=get_domain() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| sha256=metadata_artifact.sha256, pulp_domain=get_domain() | |
| sha256=metadata_artifact.sha256, pulp_domain=main_artifact.pulp_domain |
get_domain() will always return the default domain in the migration. We need this migration to work across all domains.
No description provided.