1060322 - Move hardcoded Bugzilla & slave health URLs to thUrl

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P5)

Description

[Ed Morley [:emorley]]

eg:
https://github.com/mozilla/treeherder-ui/search?q="bugzilla.mozilla.org"&type=Code

Guessing somewhere under https://github.com/mozilla/treeherder-ui/tree/master/webapp/app/js/services would be a good place to put them.

Comment 1

[Tapesh Mandal]

Hi Ed
     Sir kindly let me know as to how to determine the hardcoded URLs..kindly suggest :)

Regards and best wishes
Tapesh Mandal

Comment 2

[Jonathan French (:jfrench)]

Tapesh would like to take this one on, so assigning. Thank you Tapesh!

Comment 3

[Ed Morley [:emorley]]

(In reply to Tapesh Mandal from comment #1)
> Hi Ed
>      Sir kindly let me know as to how to determine the hardcoded
> URLs..kindly suggest :)
> 
> Regards and best wishes
> Tapesh Mandal

Hi Tapesh - are you asking what a hardcoded URL is, or the best way to find them within the source?

Comment 4

[Tapesh Mandal]

Hi Ed,
      i think hardcoded is something which remains constant..like database connection in server side scripting ..i was actually trying to find the URLs or a way to determine them ..kindly help..meanwhile i will read a bit about config files :-)

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Tapesh Mandal from comment #4)
> Hi Ed,
>       i think hardcoded is something which remains constant..like database
> connection in server side scripting ..i was actually trying to find the URLs
> or a way to determine them ..kindly help..meanwhile i will read a bit about
> config files :-)
I believe this should include any URLs that have been put directly into html files in the treeherder-ui repo. You'd probably find a decent list by searching for "https" in treeherder-ui's github page, and then filter the search for only html files. You could then check that list with Ed to make sure each item really needs to be moved.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Tapesh Mandal from comment #4)
> Hi Ed,
>       i think hardcoded is something which remains constant..like database
> connection in server side scripting ..i was actually trying to find the URLs
> or a way to determine them ..kindly help..meanwhile i will read a bit about
> config files :-)

And the reason why we'd want to do this is so there's one central place to make changes to URLs instead of having to hunt down every single instance and hope you didn't miss any.

 An example of this change will be the upcoming move of Bugherder's hosting to bugherder.mozilla.org. Once that move happens, someone will need to make sure Treeherder gets updated to point to the new location, which would be much easier if all of those URLs are specified in one config file, which is then used in the html files as needed.

This could also be useful of people wanted to test Treeherder against a different bugzilla instance. Right now with everything hardcoded to bugzilla.mozilla.org, we'd need to update many different files to point to a different bugzilla installation. If it was just a single URL in a config file, you'd only need to change that one URL to get it applied everywhere.

Comment 7

[Tapesh Mandal]

Hi Wes
      now i have started to believe in telepathy :D bcoz i searched for 'https:' in mozilla/treeherder-ui in github an hour ago..good to see we are on the same page..ok from your last comment what i understand is we need to include all the URLs in a config file so that its easy to manage/edit. what i propose is, in treeherder-ui/webapp/app/js/config there is local.conf.js..why dont we append it with rest of the URLs (41 instances according to search)?? the question is can we ??

Best Wishes,
Tapesh Mandal

Comment 8

[Jonathan French (:jfrench)]

(In reply to Tapesh Mandal from comment #7)
> what i propose is, in treeherder-ui/webapp/app/js/config there is local.conf.js

That file is manually created, and also modified by the end user. Perhaps you might wish to look through webapp/app/js/values.js, or perhaps consider a new webapp/app/js/constants.js, or similar.

Comment 9

[Tapesh Mandal]

Hi Ed
     I don't see much of repitition of URLs in https://github.com/mozilla/treeherder-ui/search?p=1&q=https&utf8=%E2%9C%93 So do you think we should move all the URLs to a config file..and as mentioned in comment 8 i think a new webapp/app/js/constants.js is better Please suggest :)

Comment 10

[Ed Morley [:emorley]]

I wouldn't rely on Github search, it only shows some results (and only the first N from each file). Use a text editor locally (or grep) and have a go at moving them somewhere. With a PR open it becomes much easier for us to comment and iterate - and half the fun of contributing for projects like these is figuring out what to do - if we list every step then there's no challenge :-)

Comment 11

[Jonathan French (:jfrench)]

(In reply to Tapesh Mandal from comment #9)
> So do you think we should move all the URLs to a config file..and as
> mentioned in comment 8 i think a new webapp/app/js/constants.js is better

Wherever you prefer is fine to start, I was just putting out some ideas. :) Like Ed says it can easily be changed or moved during review.

Comment 12

[Tapesh Mandal]

Respected all,
              My apologies for slow response..i went through my grep output and shortlisted the following URLs which i thought should find a place in the config file..They are as follows :-

1.Path: webapp/app/plugins/failure_summary/main.html   
                  /partials/main/thRelatedBugQueued.html

  URL: https://bugzilla.mozilla.org/show_bug.cgi?id={{::bug.id}}

2.Path: webapp/app/partials/main/thRelatedBugSaved.html
                  /plugins/pluginpanel.html

  URL: https://bugzilla.mozilla.org/show_bug.cgi?id={{::bug.bug_id}}

3.Path:webapp/app/partials/main_jobs.html

  URL: https://bugzilla.mozilla.org/show_bug.cgi?product=Tree%20Management&component=Treeherder

4.Path:webapp/app/plugins/pluginpanel.html
       webapp/app/dist/index.html

  URL: https://secure.pub.build.mozilla.org/builddata/reports/slave_health/slave.html?name={{job.machine_name}}


feel free to add/delete/edit the above list..any suggestion is valuable..i have only concentrated on important URLs.

Regards,
Tapesh Mandal

Comment 13

[Jonathan French (:jfrench)]

Thanks Tapesh. I'm setting Ed as mentor and allowing him to comment as he entered the bug.

If you end up having larger lists than above, you can alternately add the list as an attachment and put a needinfo on the attachment. Or if you've made the changes locally and tested them already, you can open a PR and the iterations can happen there.

Thanks!

Comment 14

[Ed Morley [:emorley]]

Yup that list looks good - I haven't checked if there others, but from memory I think that's the worst of them. If you open a PR we can iterate from there :-)

Comment 15

[Cameron Dawson [:camd]]

Tapesh: I would personally recommend making an entry in values.js for these.  Similar to one of the ``treeherder.value`` entries.  Make your own so they can be referenced in the code.  Take a look at how we reference the ``treeherder.value("thJobNavSelectors"`` from that file in the ``js/controllers/main.js`` file.  That should give you an example to work from.

Comment 16

[Ed Morley [:emorley]]

Tapesh opened a PR a while back (but isn't mentioned in this bug), which I've also been commenting on:
 https://github.com/mozilla/treeherder/pull/598

Comment 17

[Tapesh Mandal]

Hi Camd ,
        Thanks a lot :)

Regards
Tapesh Mandal

Comment 18

[Tapesh Mandal]

Hi Camd,

       Suppose if i create
 
STEP 1:

 treeherder.value("thURLs",
    {
        URL1: "http//blah.blah"
       
    }
);

STEP 2:
in js/controllers/main.js

 $Scope.getURLs= function(thURLs) {
            return thURLs.URL1
        };
STEP 3:

this is where i am confused.According to me i should first include the main.js using the script tag..then how to refer in href attribute ?? is it href={{getURLs()}}   ??

Kindly correct me wherever i have gone wrong ...

Regards,
Tapesh Mandal

Comment 19

[Jonathan French (:jfrench)]

Hi Tapesh, I defer to Ed since he's mentoring the bug. Sometimes code related discussions are best in a PR if you are far enough along to open one, it provides better context and also a testable branch for the mentor.

Comment 20

[Ed Morley [:emorley]]

Hi Tapesh, see the example I added to the pull request:
https://github.com/mozilla/treeherder/pull/598#issuecomment-107877966

Comment 21

[Cameron Dawson [:camd]]

I agree with Ed's comment in the PR for this.  Just create a ``factory`` item in services/main.js to return this info.

Comment 22

[Tapesh Mandal]

Attachment: Bug 1060322 still testing

Hi,
 
there is some problem with the changes in the pull request..but unfortunately i am not being able to find it..Kindly guide me :)

Regards,
Tapesh Mandal

Comment 23

[Jonathan French (:jfrench)]

Best practice here would be to refrain from opening the PR until after the branch is passing local unit tests and working locally. Otherwise it will just fail on CI when the PR is opened.

However now it's open, you can leave it open I think. So we don't end up with multiple PRs for the same fix (one closed unmerged, one fixed later and merged). We'd like to avoid that. Thanks! :)

Comment 24

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #19)
> Sometimes code related discussions are best in a PR if you are far enough along to open
> one, it provides better context and also a testable branch for the mentor.

Per IRC discussion to clarify, in 'far enough along' and 'testable branch' I implied a runnable feature and branch for the reviewer to evaluate. Sorry I wasn't explicit there.

Comment 25

[Tapesh Mandal]

(In reply to Jonathan French (:jfrench) from comment #24)
> (In reply to Jonathan French (:jfrench) from comment #19)
> > Sometimes code related discussions are best in a PR if you are far enough along to open
> > one, it provides better context and also a testable branch for the mentor.
> 
> Per IRC discussion to clarify, in 'far enough along' and 'testable branch' I
> implied a runnable feature and branch for the reviewer to evaluate. Sorry I
> wasn't explicit there.

no worries :D its my fault actually :)

Comment 26

[Cameron Dawson [:camd]]

Tapesh, I added some code help in the PR, please review it and see if that helps you progress on this bug.

Thanks.  :)

Comment 27

[Ed Morley [:emorley]]

Comment on attachment 8640463 [details] [review]
Bug 1060322  still testing

Clearing the review flag since it wasn't pointed at a particular person, so means that no one has it appear on their dashboard.

Also obsoleting this attachment, since it points at the now-closed #816 PR. Please can you update the existing PR (https://github.com/mozilla/treeherder/pull/598) so all of the review comments are in one place - I've left a comment explaining how to do so:
https://github.com/mozilla/treeherder/pull/598#issuecomment-126259578

Thanks :-)

Comment 28

[Tapesh Mandal]

Attachment: Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file

Respected All,
             i am thankful to everyone involved for their support and patience.

Regards
Tapesh Mandal

Comment 29

[Jonathan French (:jfrench)]

Clearing my needinfo unless there's something specific that Ed is not already covering :)

Comment 30

[Ed Morley [:emorley]]

Comment on attachment 8642524 [details] [review]
Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file

Looks good so far - well done!

There are a couple of changes that need to be made - I've left comments on the PR. If we could make those changes, update the commit with them (use --amend), rebase on latest master (https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request#how-do-i-rebase), and then force push - we should be very close to merging :-)

Comment 31

[Tapesh Mandal]

Attachment: Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file (5/8/15)

Respected All,
              I admit that i should have been careful about the syntax..i have made the required changes :)

Regards
Tapesh Mandal

Comment 32

[Ed Morley [:emorley]]

Comment on attachment 8643782 [details] [review]
Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file (5/8/15)

Have added a comment to the PR :-)
(Obsoleting this attachment since both this and the existing one point at the same PR, so we can re-use the existing attachment)

Comment 33

[Tapesh Mandal]

Hi Ed,
     This is in reference to the comment made in PR. i made the required changes :)

Regards
Tapesh Mandal

Comment 34

[Ed Morley [:emorley]]

Very close now - have added a comment to the PR :-)

Comment 35

[Tapesh Mandal]

Hi Ed ,
         I have made the changes. :)
Regards
Tapesh Mandal

Comment 36

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/9a2497df868e2fa9ad71d14c4efbe39fd3eb59e9
Bug 1060322 - Move hardcoded Bugzilla & slave health URLs to thUrl

Comment 37

[Ed Morley [:emorley]]

Comment on attachment 8642524 [details] [review]
Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file

Thank you, have merged this by hand to master :-)