1270827 - support substitution in DesupportBlob urls

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

In bug 1269811, a use case came up for supporting substitution of a few variables in DesupportBlob detailsUrls, specifically:
> https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/osx

We could probably support this without too much effort. %VERSION% and %LOCALE% can come straight from the incoming URL. %OS% might be trickier depending what the values are. If they match our %BUILD_TARGET%, it would be easy.

Comment 1

[meet123mangukiya]

I'd like to work on this bug. What are the steps to do it? I figured it has be done in `desupport.yml`

Comment 2

[[e-mail] (:bhearsum)]

(In reply to meet123mangukiya from comment #1)
> I'd like to work on this bug. What are the steps to do it? I figured it has
> be done in `desupport.yml`

Hi, thanks for your interest!

To get started, I would recommend cloning the repository and making sure you can run the tests first (instructions are at https://wiki.mozilla.org/Balrog#Hacking).

We try to take a test driven development approach, so once you're able to run the existing tests successfully, I recommend writing new tests for this, and making sure they fail. These tests should go in https://github.com/mozilla/balrog/blob/e93c3a4e41658b84bc6c8b808d86dec42104bf3e/auslib/test/blobs/test_apprelease.py#L1610

Finally, write the new code to add this functionality. You were close when you suggested it would be done in desupport.yml, but because this will be adding new logic to an existing field (instead of adding an entirely new field), the changes will be in the "DesupportBlob" class, located at https://github.com/mozilla/balrog/blob/e93c3a4e41658b84bc6c8b808d86dec42104bf3e/auslib/blobs/apprelease.py#L675.

I know this may be a lot to take in, so feel free to reach out by e-mail ([e-mail]), IRC (irc://irc.mozilla.org/#balrog), or by commenting here if you have any questions.

Comment 3

[meet123mangukiya]

Sorry, I had not been able to work on this until now, the XML presented in the docstring shows that the current state's detailsURL is fixed, and we need to make it variable, right?

And we wish to extract this information from
 
https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/osx

It is a bit unclear to me, what am I supposed to do :/

Comment 4

[meet123mangukiya]

And the URL is supposed to be changed depending on the information of the OS, browser version. Do we need to make some if else statements and change the URL accordingly?

I tried /msg bhearsum in IRC, says no such nick :/

Comment 5

[Justin Wood (:Callek)]

(In reply to meet123mangukiya from comment #4)
> And the URL is supposed to be changed depending on the information of the
> OS, browser version. Do we need to make some if else statements and change
> the URL accordingly?
> 
> I tried /msg bhearsum in IRC, says no such nick :/

Ben is mostly around on weekdays (~USA/Canadian east coast) -- however you can also e-mail him, he is pretty responsive (though not always during weekend/family time).

Either way on IRC you can usually find him in #balrog as well. (Sometimes people other than him can help there too, but he is your best resource)

Comment 6

[[e-mail] (:bhearsum)]

(In reply to meet123mangukiya from comment #4)
> And the URL is supposed to be changed depending on the information of the
> OS, browser version. Do we need to make some if else statements and change
> the URL accordingly?

That's right - detailsUrl needs to change based on information in the "updateQuery" parameter to DesupportBlob.getInnerXML. That data structure will have a "version" and a "locale" attribute that can be used to substitute the %VERSION% and %LOCALE% placeholders. You can a similar thing being done here: https://github.com/mozilla/balrog/blob/master/auslib/blobs/apprelease.py#L206

Let's ignore %OS% for now, as it will be a bit more complicated. You can come back to that once %VERSION% and %LOCALE% are working.

Comment 7

[meet123mangukiya]

Am I supposed to raise a PR in github, what's the workflow?

Comment 8

[meet123mangukiya]

Also, I'd like to know can I use {variable_name}.format(variable_name = some_name) syntax, and wouldn't it had been a better thing to use then `%s`, just a doubt that I had, obviously you guys are more experienced, I am just trying to understand the reasoning :D

Comment 9

[[e-mail] (:bhearsum)]

(In reply to meet123mangukiya from comment #7)
> Am I supposed to raise a PR in github, what's the workflow?

Yeah, a pull request is the right thing to do.

(In reply to meet123mangukiya from comment #8)
> Also, I'd like to know can I use {variable_name}.format(variable_name =
> some_name) syntax, and wouldn't it had been a better thing to use then `%s`,
> just a doubt that I had, obviously you guys are more experienced, I am just
> trying to understand the reasoning :D

You're welcome to use .format(). A lot of the code that uses %s is pretty old, and was written before .format() was the new standard.

Comment 10

[meet123mangukiya]

Attachment: apprelease.py

Please let me know if there is anything else thats needed to be added, I think I should edit the docstring as well, I'll ask for your approval for that?

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8790297 [details] [diff] [review]
apprelease.py

I'll review this here for now, but a pull request would be better for the next iteration. That will have the added benefit of running the tests for you :).

>    def getInnerXML(self, updateQuery, update_type, whitelistedDomains, specialForceHosts):
>        xml = []
>        xml.append('    <update type="%s" unsupported="true" detailsURL="%s" displayVersion="%s">' % (update_type, " https://support.mozilla.org/1/firefox/{version}/%OS%/{locale}/osx".format(version = updateQuery.version, locale = updateQuery.locale), self["displayVersion"]))
>        return xml

You're working in the right place, but there's a couple of issues:
1) We cannot hardcode URLs. You should be using self["detailsUrl"] instead of hardcoding a support.mozilla.org one.
2) I was wrong about using .format() - my apologies. We actually use %VERSION% and %LOCALE% because it's easier for humans when reading and writing the URLs. You'll need to switch to .replace().

Comment 12

[meet123mangukiya]

Yeah, If the URLs will have the %VERSION% and %LOCALE in them, then we gotta use .replace(), we got no other choice anyway. Was updating my local repo to be in sync with the remote one, so I thought I just drop the patch :D PR will be up soon

Comment 13

[meet123mangukiya]

Here is the PR https://github.com/mozilla/balrog/pull/122

Comment 14

[[github robot]]

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

https://github.com/mozilla/balrog/commit/172cf7b1fe5297f88d2fc0462f19695c2b3d6c21
bug 1270827: support substitution in DesupportBlob urls (#122). r=bhearsum

Comment 15

[[e-mail] (:bhearsum)]

https://github.com/mozilla/balrog/pull/122 has had some iteration, and been merged to master. Once it's been deployed to production (probably later this week) we'll be able to do %LOCALE% and %VERSION% substitution.

We still need support for %OS%. Benjamin, do you know what form this needs to be in, if any? It would be very easy for us to just forward %BUILD_TARGET% from the URL, but I'm not sure if SUMO needs something specific.

Comment 16

[Benjamin Smedberg]

http://searchfox.org/mozilla-central/source/toolkit/components/urlformatter/nsURLFormatter.js#112 says OS is this.appInfo.OS which is like BUILD_TARGET but without the -ABI part.

Comment 17

[[e-mail] (:bhearsum)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #16)
> http://searchfox.org/mozilla-central/source/toolkit/components/urlformatter/
> nsURLFormatter.js#112 says OS is this.appInfo.OS which is like BUILD_TARGET
> but without the -ABI part.

Looks like this is everything after the "_" in the build target, so concretely: Linux, Darwin, WINNT, Android?

Comment 18

[Benjamin Smedberg]

I can confirm WINNT and Linux on my machines, so that seems right.

Comment 19

[[e-mail] (:bhearsum)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #18)
> I can confirm WINNT and Linux on my machines, so that seems right.

Thanks!

Meet, would you like to follow-up and add support for %OS% as well? It's replacement value will be everything before the "_" in updateQuery["buildTarget"]. Eg: when buildTarget is WINNT_x86-msvc, %OS% should be WINNT.

Comment 20

[meet123mangukiya]

Sure, sorry for the late reply, actually I hadn't seen it yet. Been busy with my school exams.

Comment 21

[meet123mangukiya]

Would I be able to use the `re` module to strip everything after the "_"?

Comment 22

[[e-mail] (:bhearsum)]

(In reply to meet123mangukiya from comment #20)
> Sure, sorry for the late reply, actually I hadn't seen it yet. Been busy
> with my school exams.

No problem, you can work at whatever pace is comfortable for you.

(In reply to meet123mangukiya from comment #21)
> Would I be able to use the `re` module to strip everything after the "_"?

"re" is probably overkill. I think a simple .split() of the string should suffice.

Comment 23

[meet123mangukiya]

(In reply to Ben Hearsum (:bhearsum) from comment #22)
> (In reply to meet123mangukiya from comment #20)
> > Sure, sorry for the late reply, actually I hadn't seen it yet. Been busy
> > with my school exams.
> 
> No problem, you can work at whatever pace is comfortable for you.
> 
> (In reply to meet123mangukiya from comment #21)
> > Would I be able to use the `re` module to strip everything after the "_"?
> 
> "re" is probably overkill. I think a simple .split() of the string should
> suffice.

Certainly, it didn't come to my mind. I think this is gonna be short too ;) Gotta handle the tests again.

Comment 24

[meet123mangukiya]

https://github.com/mozilla/balrog/pull/127

Comment 25

[[github robot]]

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

https://github.com/mozilla/balrog/commit/22240a418d26b5745c6447303555cb9004afd8b3
bug 1270827: support substitution of %OS% in DesupportBlob urls (#127). r=bhearsum

Comment 26

[[e-mail] (:bhearsum)]

This is in production now, thanks for implementing it, Meet!


Benjamin or others, please let us know if we should update any of the existing desupport URLs. We point at https://support.mozilla.org/kb/your-hardware-no-longer-supported for SSE deprecation, and https://support.mozilla.org/kb/firefox-osx for 10.6-10.8 deprecation.

Comment 27

[Benjamin Smedberg]

That's a question for SUMOfolk.

Comment 28

[Patrick McClard;pmcclard]

Hello all,

We're in the process of moving the support site to a new platform. It sounds like these are changes within Firefox itself rather than the platform, but we only have one developer who's focused on the migration. I've cc'd Giorgos for his comments.

Comment 29

[Giorgos Logiotatidis [:giorgos]]

As far as SUMO is concerned the posted deprecation URLs will continue to work.