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.

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

Fix the default value of CHUNKED_UPLOAD_DIR #1078

Merged
merged 1 commit into from Jan 21, 2021
Merged

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Jan 20, 2021

This patch changes the default value of CHUNKED_UPLOAD_DIR to upload.
This value is a relative path inside the MEDIA_ROOT. Doc strings have
also been updated to match the actual behavior of functions used to
form the path used by the UploadChunk's file field. The documentation
was also updated to make it more clear that CHUNKED_UPLOAD_DIR is
supposed to be a relative path inside MEDIA_ROOT.

fixes: #8099
https://pulp.plan.io/issues/8099

Copy link
Contributor

@daviddavis daviddavis left a comment

Choose a reason for hiding this comment

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

LGTM. Needs a changelog entry.

@pulpbot
Copy link
Member

pulpbot commented Jan 20, 2021

Attached issue: https://pulp.plan.io/issues/8099

Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This is an alternative to #1074.

docs/settings.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

👍 for the actual code change. I do think the docs could steer users away from changing it.

Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

If I was my project I'd suggest the imperative mood for the commit message (see https://chris.beams.io/posts/git-commit/#imperative for the reasoning), but I'll leave that up to the actual maintainers. Resulting change looks good to me.

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I don't exactly see, how this variant survives without crying for a new migration, but as the tests are green i suppose it is correct.
I second @ekohl on the commit message style.

@daviddavis
Copy link
Contributor

I don't exactly see, how this variant survives without crying for a new migration, but as the tests are green i suppose it is correct.

It's because the field uses the upload_to callback rather than referring to the setting directly.

@dkliban dkliban changed the title Fixes the default value of CHUNKED_UPLOAD_DIR to be a relative path. Fix the default value of CHUNKED_UPLOAD_DIR Jan 20, 2021
@ekohl
Copy link
Contributor

ekohl commented Jan 20, 2021

What I would write as the commit message:

Make CHUNKED_UPLOAD_DIR a relative path

In 1b6c736 uploads were changed to use the default storage (uses settings.MEDIA_ROOT). Anything that's written outside of storage location raises a SuspiciousOperation. That already made the implicit requirement that CHUNKED_UPLOAD_DIR was relative.

Users could hit this if they modified MEDIA_ROOT in their settings but kept CHUNKED_UPLOAD_DIR default.

If a relative path is used, Django prepends the location and it is guaranteed to be a safe location. This changes the default value to be relative and updates the documentation to reflect this.

@daviddavis
Copy link
Contributor

Actually now that I think of it: I wonder if we ought to have a .removal to say that CHUNKED_UPLOAD_DIR should be a relative path now instead of an absolute path?

@dkliban
Copy link
Member Author

dkliban commented Jan 20, 2021

Actually now that I think of it: I wonder if we ought to have a .removal to say that CHUNKED_UPLOAD_DIR should be a relative path now instead of an absolute path?

We could. We already had a .removal related to this setting for the 3.9.0 release.

https://docs.pulpproject.org/pulpcore/changes.html#removals

In 1b6c736 uploads were changed to use the default storage (uses
settings.MEDIA_ROOT). Anything that's written outside of storage
location raises a SuspiciousOperation. That already made the implicit
requirement that CHUNKED_UPLOAD_DIR was relative.

Users could hit this if they modified MEDIA_ROOT in their settings
but kept CHUNKED_UPLOAD_DIR default.

If a relative path is used, Django prepends the location and it is
guaranteed to be a safe location. This changes the default value to be
relative and updates the documentation to reflect this.

fixes: #8099
https://pulp.plan.io/issues/8099
@mdellweg
Copy link
Member

Two (green) approvals; all tests passed; no migration needed; last minute comments addressed
I am going to play merge bot now.

@mdellweg mdellweg merged commit a805312 into pulp:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants