Closed
Bug 1268088
Opened 8 years ago
Closed 8 years ago
[beetmover] Set Content-Type for uploaded files
Categories
(Release Engineering :: Release Automation: Other, defect)
Release Engineering
Release Automation: Other
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rail, Assigned: csheehan)
References
Details
Attachments
(1 file, 4 obsolete files)
2.25 KB,
patch
|
rail
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
It'd be great to set Content-Type for text files, such as checksums, to text/plain instead of application/octet-stream. I'm not sure if the user has enough powers to set those though.
Reporter | ||
Comment 1•8 years ago
|
||
http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/scripts/release/beet_mover.py#l304 is where we upload files. We can use builtin `mimetypes` module, but explicitly specify file types used by us. You can find all the types under one of the candidates: http://ftp.mozilla.org/pub/firefox/candidates/47.0b4-candidates/
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → csheehan
Assignee | ||
Comment 2•8 years ago
|
||
I ran through the list of files in the checksums and made a list of all the different file extensions. Then I found all of the extensions that weren't defined in the mimetypes packages and mapped them to a type (in the dict MIME_MAP). Then I made a function mime_fix that initializes the module and adds the new mappings, and called mime_fix at the bottom of the BeetMover constructor. Finally in upload_bit I rewrote the call to self.retry to look more readable, and added the header to the call.
Attachment #8751419 -
Flags: feedback?(jlund)
Comment 3•8 years ago
|
||
Comment on attachment 8751419 [details] [diff] [review] beetmover.diff Review of attachment 8751419 [details] [diff] [review]: ----------------------------------------------------------------- nice use of lambda and map. much cleaner. now here is hoping we have permissions to set these headers :D TIL - heh, I didn't realize that items generated from .values() and keys() always created consistent order. since at definition time, items in a dict don't preserve order, I never really thought that those iterator methods would be consistent there after. :) ::: testing/mozharness/scripts/release/beet_mover.py @@ +108,5 @@ > ] > CACHE_DIR = 'cache' > > +MIME_MAP = { > + '': 'text/plain', were you running into issues without this? does this act as a default/fallback or something? @@ +351,5 @@ > return any(re.search(exclude, keyname) for exclude in self.excludes) > > + def mime_fix(self): > + """ Add mimetypes for custom extensions """ > + mimetypes.init() huh, the module asks us to init() itself? it looks kind of funny. seems like something this mimetypes should do under the hood.
Attachment #8751419 -
Flags: feedback?(jlund) → feedback+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jordan Lund (:jlund) from comment #3) > Comment on attachment 8751419 [details] [diff] [review] > beetmover.diff > > Review of attachment 8751419 [details] [diff] [review]: > ----------------------------------------------------------------- > > nice use of lambda and map. much cleaner. now here is hoping we have > permissions to set these headers :D Thanks! :) > > TIL - heh, I didn't realize that items generated from .values() and keys() > always created consistent order. since at definition time, items in a dict > don't preserve order, I never really thought that those iterator methods > would be consistent there after. :) > > ::: testing/mozharness/scripts/release/beet_mover.py > @@ +108,5 @@ > > ] > > CACHE_DIR = 'cache' > > > > +MIME_MAP = { > > + '': 'text/plain', > > were you running into issues without this? does this act as a > default/fallback or something? Yes, that was the reasoning. Just in case. > > @@ +351,5 @@ > > return any(re.search(exclude, keyname) for exclude in self.excludes) > > > > + def mime_fix(self): > > + """ Add mimetypes for custom extensions """ > > + mimetypes.init() > > huh, the module asks us to init() itself? it looks kind of funny. seems like > something this mimetypes should do under the hood. Haha, agreed. And if you don't call init before using one of the helper methods, it gets called anyways. Thanks again for your feedback!
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8751419 [details] [diff] [review] beetmover.diff Review of attachment 8751419 [details] [diff] [review]: ----------------------------------------------------------------- I couple of nits: ::: testing/mozharness/scripts/release/beet_mover.py @@ +313,5 @@ > self.info("Uploading to `{}`".format(s3_key)) > key = bucket.new_key(s3_key) > # set key value > + headers = mimetypes.guess_type(source) > + self.retry(lambda: key.set_contents_from_filename(source, headers=headers[0]), error_level=FATAL), You need to construct the header, not just pass the value. See https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/release/generate-checksums.py#263 @@ +352,5 @@ > > + def mime_fix(self): > + """ Add mimetypes for custom extensions """ > + mimetypes.init() > + map(mimetypes.add_type, MIME_MAP.values(), MIME_MAP.keys()) I would not rely on consistent dictionary sorting and would use something like map(mimetypes.add_type, MIME_MAP.items())
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Jordan Lund (:jlund) from comment #3) > nice use of lambda and map. much cleaner. now here is hoping we have > permissions to set these headers :D We should be able to set them, see https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/release/generate-checksums.py#263
Assignee | ||
Comment 7•8 years ago
|
||
Fixed the header definition, changed the mapping to use MIME_MAP.items() for guaranteed consistency and moved away from map as it was becoming less clear than a simple loop.
Attachment #8751419 -
Attachment is obsolete: true
Attachment #8752297 -
Flags: feedback?(rail)
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8752297 [details] [diff] [review] beetmover_2.diff Review of attachment 8752297 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozharness/scripts/release/beet_mover.py @@ +313,4 @@ > self.info("Uploading to `{}`".format(s3_key)) > key = bucket.new_key(s3_key) > # set key value > + headers = mimetypes.guess_type(source) the variable name is a bit confusing. I'd probably use something like: mime_type, _ = mimetypes.guess_type(source) @@ +353,5 @@ > > + def mime_fix(self): > + """ Add mimetypes for custom extensions """ > + mimetypes.init() > + for ext, type in MIME_MAP.items(): mimetypes.add_type(type, ext) a nit: type is a builtin function, better to not override it. another nit: disk space is cheap ;), add a new line after the colon.
Attachment #8752297 -
Flags: feedback?(rail) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
Made a few changes according to Rail's suggestions.
Attachment #8752297 -
Attachment is obsolete: true
Attachment #8752304 -
Flags: feedback?(jlund)
Comment 10•8 years ago
|
||
Comment on attachment 8752304 [details] [diff] [review] beetmover_3.diff Review of attachment 8752304 [details] [diff] [review]: ----------------------------------------------------------------- close, f+ but need to address my below comment :) ::: testing/mozharness/scripts/release/beet_mover.py @@ +353,5 @@ > > + def mime_fix(self): > + """ Add mimetypes for custom extensions """ > + mimetypes.init() > + map(lambda ext, mime_type: mimetypes.add_type(mime_type, ext), MIME_MAP.items()) hm, correct me if I am wrong but I think items() will give you back a list of key/pair sets: [(key, pair), ..etc]. map() will pass each of those sets as a single arg for your lambda. But your lambda expects two args
Attachment #8752304 -
Flags: feedback?(jlund) → feedback+
Assignee | ||
Comment 11•8 years ago
|
||
Ahhh you are absolutely right, I meant to unpack the tuple into those two variable names. Hopefully now it is better.
Attachment #8752304 -
Attachment is obsolete: true
Attachment #8752920 -
Flags: feedback?(jlund)
Comment 12•8 years ago
|
||
Comment on attachment 8752920 [details] [diff] [review] beetmover_lambdafix.diff Review of attachment 8752920 [details] [diff] [review]: ----------------------------------------------------------------- looks right to me :)
Attachment #8752920 -
Flags: feedback?(jlund) → feedback+
Reporter | ||
Comment 13•8 years ago
|
||
I pushed it to jamun to test for reals! https://hg.mozilla.org/projects/jamun/rev/9b2c407f3cba
Assignee | ||
Comment 14•8 years ago
|
||
Add .beet and .checksums types to MIME_MAP (both set to text/plain).
Attachment #8752920 -
Attachment is obsolete: true
Attachment #8752983 -
Flags: review?(rail)
Reporter | ||
Updated•8 years ago
|
Attachment #8752983 -
Flags: review?(rail) → review+
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8752983 [details] [diff] [review] beetmover_more_mtypes.diff https://hg.mozilla.org/integration/mozilla-inbound/rev/9f102edf659e aurora: closed, todo https://hg.mozilla.org/releases/mozilla-beta/rev/d92d5520191e https://hg.mozilla.org/releases/mozilla-release/rev/b31266ba672a https://hg.mozilla.org/releases/mozilla-esr45/rev/9cc65cca1f71
Attachment #8752983 -
Flags: checked-in+
Reporter | ||
Updated•8 years ago
|
Keywords: leave-open
Reporter | ||
Comment 17•8 years ago
|
||
Still need to land this to aurora.
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8752983 [details] [diff] [review] beetmover_more_mtypes.diff aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/0b344441f45b
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Rail Aliiev [:rail] from comment #18) > Comment on attachment 8752983 [details] [diff] [review] > beetmover_more_mtypes.diff > > aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/0b344441f45b Safe to mark this as resolved?
Flags: needinfo?(rail)
Reporter | ||
Comment 20•8 years ago
|
||
Yup. I spot checked files in http://ftp.mozilla.org/pub/firefox/candidates/47.0b6-candidates/build1/ and they look as expected.
Flags: needinfo?(rail)
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9f102edf659e
Comment 22•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•