[beetmover] Set Content-Type for uploaded files

RESOLVED FIXED

Status

Release Engineering
Release Automation: Other
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: rail, Assigned: sheehan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
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

2 years ago
Assignee: nobody → csheehan
(Assignee)

Comment 2

2 years ago
Created attachment 8751419 [details] [diff] [review]
beetmover.diff

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

2 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

2 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!
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())
(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

2 years ago
Created attachment 8752297 [details] [diff] [review]
beetmover_2.diff

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)
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

2 years ago
Created attachment 8752304 [details] [diff] [review]
beetmover_3.diff

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

2 years ago
Created attachment 8752920 [details] [diff] [review]
beetmover_lambdafix.diff

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+
(Assignee)

Comment 14

2 years ago
Created attachment 8752983 [details] [diff] [review]
beetmover_more_mtypes.diff

Add .beet and .checksums types to MIME_MAP (both set to text/plain).
Attachment #8752920 - Attachment is obsolete: true
Attachment #8752983 - Flags: review?(rail)
Attachment #8752983 - Flags: review?(rail) → review+
Keywords: leave-open
Still need to land this to aurora.
(Assignee)

Comment 19

2 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)
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

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