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)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rail, Assigned: csheehan)

References

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → csheehan
Attached 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+
(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
Attached 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)
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+
Attached 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+
Attached 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+
I pushed it to jamun to test for reals! https://hg.mozilla.org/projects/jamun/rev/9b2c407f3cba
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.
(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)
Status: NEW → RESOLVED
Closed: 8 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.