1136164 - Consider case insensitive interpolation in release blobs

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P3)

Description

[Rail Aliiev [:rail]]

See bug 1136143 for the detail.

The release blob used %locale% instead of %LOCALE% what cased not ideal update UX.

We should somehow prevent this in the future:

* use case insensitive interpolation (this may add extra load)
* validate the blobs

Comment 1

[Anjul Tyagi]

Can I work on this one?

Comment 2

[A.S.]

Surely, go ahead. Should I formally assign it to you ? Feel free to post your doubts here or ping me on IRC for any help.

Comment 3

[Anjul Tyagi]

No, it's fine. Maybe you can assign it to me after I submit a patch.

Comment 4

[Anjul Tyagi]

Please verify what is required out of this bug:

=> Add an extra check for string replacements statements like: 

   self['detailsUrl'].replace("%LOCALE%", updateQuery['locale'])
   
   inside the apprelease.py to accommodate replacement of %locale% as well.

=> Do the above check for all the variables i.e. OS_FTP, LOCALE, FILENAME, PRODUCT and OS_BOUNCER.

=> I checked for replacement statements in other blob files but only apprelease.py file has this. So, changes are only required in apprelease.py file.

=> Also, I'm not sure what does validate the blobs mean in this case, does it mean I've to put some test cases with "locale" (for example) in lower case and check whether it is validating correctly?

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Anjul Tyagi from comment #4)
> Please verify what is required out of this bug:
> 
> => Add an extra check for string replacements statements like: 
> 
>    self['detailsUrl'].replace("%LOCALE%", updateQuery['locale'])
>    
>    inside the apprelease.py to accommodate replacement of %locale% as well.

Yup. Also %Locale% or other variants.

> => Do the above check for all the variables i.e. OS_FTP, LOCALE, FILENAME,
> PRODUCT and OS_BOUNCER.

Yeah, these should be case insensitive too.

> 
> => I checked for replacement statements in other blob files but only
> apprelease.py file has this. So, changes are only required in apprelease.py
> file.

Good work! It looks that way to me, too.

> 
> => Also, I'm not sure what does validate the blobs mean in this case, does
> it mean I've to put some test cases with "locale" (for example) in lower
> case and check whether it is validating correctly?

In comment #0, "validate the blobs" was another idea about how to fix the problem Rail encountered. You can ignore that for the purpose of this bug (we do blob validation already). However, please be sure to add new test cases to https://github.com/mozilla/balrog/blob/master/auslib/test/blobs/test_apprelease.py

Comment 6

[Anjul Tyagi]

Submitted a PR: https://github.com/mozilla/balrog/pull/273

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/190e2f138c60c54f0442137ad1f006ccb0d3f481
Fixing Bug:1136164. Case insensitive interpolation of apprelease blobs. (#273). r=bhearsum,rail

Comment 8

[[e-mail] (:bhearsum)]

This made it to production today, nice work Anjul!