-
Notifications
You must be signed in to change notification settings - Fork 81
[PULP-731] Add synchronous upload API #942
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
Conversation
dcac506 to
86c2977
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.
This looks really good. A few changes and we can merge.
pulp_python/app/serializers.py
Outdated
| # e.g. '/var/lib/pulp/tmp/tmphexlltu1.upload.gz' | ||
| data["relative_path"] = vars(metadata)["filename"] | ||
| # e.g. 'shelf-reader-0.1.tar.gz' instead of '/var/lib/pulp/tmp/tmphexlltu1.upload.gz' | ||
| data["filename"] = file.name |
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.
relative_path and filename should both be the same. They should both be file.name, just the name of the file, not 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.
IMO the name ought to be canonicalized ideally, and pulled from the metadata and nothing related to the upload request. We had problems in the past with user-provided relative_path / filename in the RPM plugin.
But agree that they absolutely need to be the same value, we also had problems with that...
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.
@dralley Metadata provides only the name and version, so I can take the filename extension from file.name and use the metadata values like f"{metadata["name"]}-{metadata["version"]}{extension}". Or I can at least check that the name and version from metadata are included in file.name. How does this sound?
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.
It seems to be doable to create a file name based on the package data only (https://packaging.python.org/en/latest/specifications/section-distribution-formats/), but this would require too much work for now. We can revisit it later if needed.
EDIT: I created PULP-760 to track this.
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.
Sounds good. I just mention it because it's best to not rely on user-provided data when constructing metadata. But that's the current state of things too, I agree it's a separate issue.
3cfc249 to
9ad386a
Compare
8d0a351 to
3df3095
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.
Looks good! LGTM
closes #933