[beetmover] Set Content-Type for uploaded files

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: rail, Assigned: csheehan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

3 years ago
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

3 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

3 years ago
Assignee: nobody → csheehan
Assignee

Comment 2

3 years ago
Posted patch beetmover.diff (obsolete) — Splinter Review
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 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

3 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

3 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

3 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

3 years ago
Posted patch beetmover_2.diff (obsolete) — Splinter Review
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

3 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

3 years ago
Posted patch beetmover_3.diff (obsolete) — Splinter Review
Made a few changes according to Rail's suggestions.
Attachment #8752297 - Attachment is obsolete: true
Attachment #8752304 - Flags: feedback?(jlund)
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

3 years ago
Posted patch beetmover_lambdafix.diff (obsolete) — Splinter Review
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 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

3 years ago
I pushed it to jamun to test for reals! https://hg.mozilla.org/projects/jamun/rev/9b2c407f3cba
Assignee

Comment 14

3 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

3 years ago
Attachment #8752983 - Flags: review?(rail) → review+
Reporter

Updated

3 years ago
Keywords: leave-open
Reporter

Comment 17

3 years ago
Still need to land this to aurora.
Assignee

Comment 19

3 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

3 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

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.