Consider case insensitive interpolation in release blobs

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: rail, Assigned: anjul.ten)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=python][ready][good first bug])

(Reporter)

Description

4 years ago
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
Component: Balrog: Frontend → Balrog: Backend
Priority: -- → P3
Whiteboard: [lang=python][ready]
Whiteboard: [lang=python][ready] → [lang=python][ready][good first bug]
(Assignee)

Comment 1

2 years ago
Can I work on this one?
Flags: needinfo?(bhearsum)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
No, it's fine. Maybe you can assign it to me after I submit a patch.
Assignee: nobody → anjul.ten
Flags: needinfo?(bhearsum)
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(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
Flags: needinfo?(bhearsum)
(Assignee)

Comment 6

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

Comment 7

2 years ago
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
Depends on: 1345586
This made it to production today, nice work Anjul!
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.