Last Comment Bug 1270827 - support substitution in DesupportBlob urls
: support substitution in DesupportBlob urls
Status: RESOLVED FIXED
[lang=python][good first bug]
:
Product: Release Engineering
Classification: Other
Component: Balrog: Backend (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: meet123mangukiya
: Ben Hearsum (:bhearsum)
:
Mentors: Ben Hearsum (:bhearsum)
Depends on: 1305703
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-06 06:04 PDT by Ben Hearsum (:bhearsum)
Modified: 2016-10-04 06:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
apprelease.py (31.52 KB, patch)
2016-09-12 08:18 PDT, meet123mangukiya
bhearsum: review-
Details | Diff | Splinter Review

Description User image Ben Hearsum (:bhearsum) 2016-05-06 06:04:43 PDT
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 User image meet123mangukiya 2016-09-07 00:36:25 PDT
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 User image Ben Hearsum (:bhearsum) 2016-09-07 05:39:54 PDT
(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 (bhearsum@mozilla.com), IRC (irc://irc.mozilla.org/#balrog), or by commenting here if you have any questions.
Comment 3 User image meet123mangukiya 2016-09-10 06:19:10 PDT
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 User image meet123mangukiya 2016-09-10 06:23:17 PDT
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 User image Justin Wood (:Callek) 2016-09-10 07:38:43 PDT
(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 User image Ben Hearsum (:bhearsum) 2016-09-12 07:05:31 PDT
(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 User image meet123mangukiya 2016-09-12 08:09:30 PDT
Am I supposed to raise a PR in github, what's the workflow?
Comment 8 User image meet123mangukiya 2016-09-12 08:12:19 PDT
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 User image Ben Hearsum (:bhearsum) 2016-09-12 08:17:14 PDT
(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 User image meet123mangukiya 2016-09-12 08:18:24 PDT
Created attachment 8790297 [details] [diff] [review]
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 User image Ben Hearsum (:bhearsum) 2016-09-12 08:42:12 PDT
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 User image meet123mangukiya 2016-09-12 09:03:51 PDT
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 User image meet123mangukiya 2016-09-12 09:43:10 PDT
Here is the PR https://github.com/mozilla/balrog/pull/122
Comment 14 User image [github robot] 2016-09-12 12:37:39 PDT
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 User image Ben Hearsum (:bhearsum) 2016-09-12 12:43:32 PDT
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 User image Benjamin Smedberg [:bsmedberg] 2016-09-13 12:11:36 PDT
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 User image Ben Hearsum (:bhearsum) 2016-09-14 07:47:13 PDT
(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 User image Benjamin Smedberg [:bsmedberg] 2016-09-14 08:01:31 PDT
I can confirm WINNT and Linux on my machines, so that seems right.
Comment 19 User image Ben Hearsum (:bhearsum) 2016-09-14 13:15:27 PDT
(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 User image meet123mangukiya 2016-09-19 05:17:32 PDT
Sure, sorry for the late reply, actually I hadn't seen it yet. Been busy with my school exams.
Comment 21 User image meet123mangukiya 2016-09-19 05:23:03 PDT
Would I be able to use the `re` module to strip everything after the "_"?
Comment 22 User image Ben Hearsum (:bhearsum) 2016-09-19 05:54:56 PDT
(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 User image meet123mangukiya 2016-09-19 08:11:46 PDT
(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 User image meet123mangukiya 2016-09-20 07:30:08 PDT
https://github.com/mozilla/balrog/pull/127
Comment 25 User image [github robot] 2016-09-20 13:35:19 PDT
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 User image Ben Hearsum (:bhearsum) 2016-09-28 10:49:44 PDT
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 User image Benjamin Smedberg [:bsmedberg] 2016-09-28 10:53:02 PDT
That's a question for SUMOfolk.
Comment 28 User image Patrick McClard;pmcclard 2016-10-04 05:44:50 PDT
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 User image Giorgos Logiotatidis [:giorgos] 2016-10-04 06:03:25 PDT
As far as SUMO is concerned the posted deprecation URLs will continue to work.

Note You need to log in before you can comment on or make changes to this bug.