The default bug view has changed. See this FAQ.

support substitution in DesupportBlob urls

RESOLVED FIXED

Status

Release Engineering
Balrog: Backend
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: bhearsum, Assigned: meet123mangukiya, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
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.
(Reporter)

Updated

7 months ago
Whiteboard: [lang=python][good first bug]
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

7 months ago
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

7 months ago
(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)
(Reporter)

Comment 6

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

Comment 7

7 months ago
Am I supposed to raise a PR in github, what's the workflow?
(Assignee)

Comment 8

7 months ago
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
(Reporter)

Comment 9

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

Comment 10

7 months ago
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?
Attachment #8790297 - Flags: review?(bhearsum)
(Reporter)

Comment 11

7 months ago
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().
Attachment #8790297 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 12

7 months ago
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
(Assignee)

Comment 13

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

Comment 14

7 months ago
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
(Reporter)

Comment 15

7 months ago
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.
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
(Reporter)

Comment 17

7 months ago
(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?
Flags: needinfo?(benjamin)
I can confirm WINNT and Linux on my machines, so that seems right.
Flags: needinfo?(benjamin)
(Reporter)

Comment 19

7 months ago
(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.
Flags: needinfo?(meet123mangukiya)
(Assignee)

Comment 20

6 months ago
Sure, sorry for the late reply, actually I hadn't seen it yet. Been busy with my school exams.
(Assignee)

Comment 21

6 months ago
Would I be able to use the `re` module to strip everything after the "_"?
(Reporter)

Comment 22

6 months ago
(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.
(Assignee)

Comment 23

6 months ago
(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.
(Reporter)

Updated

6 months ago
Assignee: nobody → meet123mangukiya
Flags: needinfo?(meet123mangukiya)
(Assignee)

Comment 24

6 months ago
https://github.com/mozilla/balrog/pull/127

Comment 25

6 months ago
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
(Reporter)

Updated

6 months ago
Depends on: 1305703
(Reporter)

Comment 26

6 months ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
That's a question for SUMOfolk.
Flags: needinfo?(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.
Flags: needinfo?(pmcclard) → needinfo?(giorgos)
As far as SUMO is concerned the posted deprecation URLs will continue to work.
Flags: needinfo?(giorgos)
You need to log in before you can comment on or make changes to this bug.