Closed Bug 1056877 Opened 10 years ago Closed 9 years ago

Merge the treeherder-ui and treeherder-service repos

Categories

(Tree Management :: Treeherder, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(4 files, 1 obsolete file)

Currently, the treeherder-service Vagrantfile depends on the treeherder-ui repo out of the box - and only at a hardcoded location - which is not mentioned by the installation instructions.

There are a bunch of bugs open to make this situation better:

Bug 1045090 - Update the documentation to reflect the treeherder-ui dependency
Bug 1056865 - Vagrant: Add a local vagrant config file that is excluded by .gitignore
Bug 1056868 - Vagrant: Add ability to specify a custom directory location for treeherder-ui
Bug 1056872 - Vagrant: Change the default Vagrantfile config to not rely on the treeherder-ui repo

However, I'm wondering whether we've reached a point where it makes more sense to accept that the service and the UI are pretty closely related and so should be merged into the same repo?

IMO this would also ease development for new/less active contributors, since only one repo to clone, easier to grep for changes across both repos & also impossible for incompatible ui + service changes to be made out of sync.

If we decided we were going to do this, I imagine we would:
1) Import the ui repo into treeherder-service, preserving git history & using an appropriate location within the repo.
2) Update repo docs/deploy scripts/wiki pages/IT scripts and docs.
3) Delete the ui repo on Github
4) (Optionally) rename treeherder-service to treeherder.

We would unfortunately break links to treeherder-ui commits on Github, with no way to redirect (sadly one of the drawbacks of a repo not hosted by Mozilla), but I think now is the time to take the hit if we're going to do this - not later, when there's more broken URLs at stake.

What are your thoughts?
Flags: needinfo?(mdoglio)
Flags: needinfo?(jeads)
Flags: needinfo?(cdawson)
I don't have any objections to moving to a single repo.  We've talked about doing it before.  I think the concern was that we may want to deploy the UI on a separate machine from the service.  However, not clear we will ever do that.

It's a good point about keeping things in sync between service and ui.  We often submit "sister" PRs on service and ui where both are required to be merged prior to deployment.  So merging them would simplify things.
Flags: needinfo?(cdawson)
I'm not particularly against the merge, but I think the idea behind the separation was that UI and service don't share any code. UI development could be done using one of the service deployment on dev/stage/prod, with only authentication-related implementations requiring the two repositories to be on the same host.
Here are my doubts about the merge:
- we need to find a solution to run both python and js tests on travis. I have a couple of ideas about it but it would require some testing
- reviewing code will likely be tricky for big changes. we could probably mitigate that with smaller pull requests.
Flags: needinfo?(mdoglio)
Enabling completely separated UI development without requiring treeherder-service is a good thing. In production this also allows us to deploy the service on worker nodes for log parsing without the UI. I think we should keep them separate but add a local vagrant config file as suggested in Bug 1056865
Flags: needinfo?(jeads)
I agree the two shouldn't share code - but from a hacking/discovery POV, having both in the same repo (one under a 'service' directory and the other under 'ui') would make that easier + the landing of synced commits etc.
Blocks: 1072681
No longer blocks: 1072681
Component: Treeherder → Treeherder: Docs & Development
Priority: P2 → P3
Depends on: 1133603
I think there are many advantages to doing this. I had typed out a list but my awful lenovo keyboard shortcuts right next to the arrow keys (which I've now removed after hitting them for the umpteenth time this last week or so without my dock) caused the tab to reset in such a way that it overwrote history and I couldn't navigate back to preserve the page, sigh.

(In reply to Cameron Dawson [:camd] from comment #1)
> I don't have any objections to moving to a single repo.  We've talked about
> doing it before.  I think the concern was that we may want to deploy the UI
> on a separate machine from the service.  However, not clear we will ever do
> that.

We can still do this, we'll just clone the full combined repo on both machines.

> It's a good point about keeping things in sync between service and ui.  We
> often submit "sister" PRs on service and ui where both are required to be
> merged prior to deployment.  So merging them would simplify things.

Yeah I think this would be a great win :-)

(In reply to Mauro Doglio [:mdoglio] from comment #2)
> I'm not particularly against the merge, but I think the idea behind the
> separation was that UI and service don't share any code. UI development
> could be done using one of the service deployment on dev/stage/prod, with
> only authentication-related implementations requiring the two repositories
> to be on the same host.

We can still keep the UI and service parts separate and not reliant on each other, they'll just happen to be in the same directory (with a suitable subdirectory structure). Greps will be easier, as will Github repo search.

> Here are my doubts about the merge:
> - we need to find a solution to run both python and js tests on travis. I
> have a couple of ideas about it but it would require some testing

Agree, we need to check this.

> - reviewing code will likely be tricky for big changes. we could probably
> mitigate that with smaller pull requests.

I'd actually say the opposite: at worst it will be the same (we'll have two PRs, one for UI changes and one for service changes, we don't need two repos for that. At best, it will mean it's easier to see that the UI changes match the service changes. And smaller PRs are always a good thing regardless :-)

Plus looks like renaming the treeherder-service repo to treeherder won't break links, since they redirect both web traffic and git push/pulls:
https://help.github.com/articles/renaming-a-repository/

And now with bug 1133603 fixed, treeherder-service isn't a 400MB clone (more like 70MB) for people using the Github for Mac/Windows client or those who have the custom config to let them check out PRs - so we don't need a clean start from that repo.
I don't have as much stake here as everyone who answered above but having this would really lower the barrier for writing/maintaining good testing from the UI through the backend (recently I had to do this and testing manually is pretty painful and error prone)

so +1
Let's just do this :-)
Priority: P3 → P2
Summary: Decide if we should merge the treeherder-ui and treeherder-service repos → Merge the treeherder-ui and treeherder-service repos
Priority: P2 → P3
An additional bonus would be that we could eliminate the "grunt build" step and instead use something like django-compressor for asset merging.

https://github.com/django-compressor/django-compressor

I'd really like to see this happen sooner than later.
(In reply to William Lachance (:wlach) from comment #8)
> An additional bonus would be that we could eliminate the "grunt build" step
> and instead use something like django-compressor for asset merging.
> 
> https://github.com/django-compressor/django-compressor

Agree; bug 1151803 filed for this :-)
Priority: P3 → P2
Rough plan:
1) [DONE] On github, rename "treeherder-service" -> "treeherder". Github sets up redirects for both HTTP and git traffic, so everything will continue to work. This will mean we can stop using "treeherder-service" as directory names for things like our Heroku testing, to avoid it being hardcoded everywhere, like it is with our current infra.
2) In all wiki/readthedocs/readme/other links s#github.com/mozilla/treeherder-service#github.com/mozilla/treeherder#g
3) Decide on an appropriate repo layout for when the treeherder-ui files are imported into 'treeherder'. This doesn't need to be perfect initially, but we need to at least avoid naming clashes/confusion in the meantime.
4) Figure out git commands required to move treeherder-ui into treeherder as a subdirectory of it (eg http://bpeirce.me/moving-one-git-repository-into-another.html or http://gbayer.com/development/moving-files-from-one-git-repository-to-another-preserving-history/) + remaining file moves to convert result of this into the layout agreed in #3.
5) Work out what needs updating for the new file locations (eg stage/prod apache configs + in-repo puppet files + re-provision dev etc etc)
6) Once #3-5 figured out, stop making commits against the UI repo & perform the import in #4.
7) Work with fubar to update the stage/prod apache configs per #5 (we'll leave the treeherder-ui repo as is for this part, so we can gracefully transition)
8) Once we're using the UI from the treeherder repo, replace the contents of the UI repo with a README pointing at the other repo - and allow people to continue making UI changes (but against the other repo now).
9) Update the wiki page / readme / installation steps on readthedocs / deploy script for a single-repo model
10) Remove the now unused treeherder-ui repo/references from stage/prod
11) Sort out dev

Have I missed anything?
https://github.com/mozilla/treeherder/search?q=treeherder-ui
https://github.com/mozilla/treeherder-ui/search?q=treeherder-ui

[emorley@treeherder1.stage.webapp.scl3 ~]$ grep treeherder-ui /etc/httpd/mozilla/domains/treeherder.allizom.org.conf
  #Alias /ui /data/www/treeherder.allizom.org/treeherder-ui/dist
  Alias /help.html /data/www/treeherder.allizom.org/treeherder-ui/dist/help.html
  Alias /logviewer.html /data/www/treeherder.allizom.org/treeherder-ui/dist/logviewer.html
  Alias /perf.html /data/www/treeherder.allizom.org/treeherder-ui/dist/perf.html
  Alias /js /data/www/treeherder.allizom.org/treeherder-ui/dist/js
  Alias /css /data/www/treeherder.allizom.org/treeherder-ui/dist/css
  Alias /img /data/www/treeherder.allizom.org/treeherder-ui/dist/img
  Alias /fonts /data/www/treeherder.allizom.org/treeherder-ui/dist/fonts
# # These locations are to support loading files directly from treeherder-ui
# Alias /vendor /data/www/treeherder.allizom.org/treeherder-ui/<%= @APP_UI %>/vendor
# Alias /plugins /data/www/treeherder.allizom.org/treeherder-ui/<%= @APP_UI %>/plugins
# Alias /partials /data/www/treeherder.allizom.org/treeherder-ui/<%= @APP_UI %>/partials
# Alias /icons /data/www/treeherder.allizom.org/treeherder-ui/<%= @APP_UI %>/icons
  Alias / /data/www/treeherder.allizom.org/treeherder-ui/dist/index.html
The current layout is:

treeherder-service/bin/
treeherder-service/build/
treeherder-service/deployment/
treeherder-service/docker/
treeherder-service/docs/
treeherder-service/puppet/
treeherder-service/requirements/
treeherder-service/schemas/
treeherder-service/tests/
treeherder-service/treeherder/
treeherder-service/vendor/

treeherder-ui/dist/
treeherder-ui/docs/
treeherder-ui/webapp/
treeherder-ui/webapp/app/
treeherder-ui/webapp/config/
treeherder-ui/webapp/logs/
treeherder-ui/webapp/scripts/
treeherder-ui/webapp/test/

I guess we can (eventually) combine 'docs'.

I've never really been a fan of the current structure under 'treeherder-ui/webapp/'.

Does anyone have a preference/suggestions for this?
I would put the content of webapp at the root of the repo and remove webapp.
(In reply to Ed Morley [:emorley] from comment #10)
> Rough plan:
> Have I missed anything?

Perhaps in the days preceding the merge, when joining IRC #treeherder (a login notice?) and/or on the project Wiki, we can announce the upcoming change and include guidance for contributors or those with open PRs.
Assignee: nobody → emorley
First pass; the rest (eg docs referring to "service vs UI" only makes sense to do once we've combined the repos).
Attachment #8594811 - Flags: review?(cdawson)
Attachment #8594811 - Flags: review?(cdawson) → review+
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/8478629ef4133fbaf224ce58ee4e40627e5fb337
Bug 1056877 - Update links & puppet/Vagrant config for new repo name

The 'treeherder-service' repo has been renamed to 'treeherder', ready
for when the treeherder-ui repo is imported into it. This means the
Github URL, Travis URL and directory name when cloned changes. The Read
The Docs URL cannot be changed, so for now we will leave as-is, and in
the future (once service and UI docs combined) we will create a new
project on RTD with name "treeherder".

This updates doc links and puppet/Vagrant configs, but leaves the
stage/prod deploy script alone, since renaming the directories on our
infra is non-trivial. The dev instance will need some TLC since unlike
stage/prod, it does use the puppet scripts in the repo.
Note: the commit in comment 16 will mean people need to re-provision their Vagrant environment and/or destroy and re-up - not really any easy way to avoid that unfortunately.
Attachment #8594811 - Flags: checkin+
Blocks: 1076886
Trying to rearrange the repo structure significantly at the same time as merging is turning out to be too much of a pain.

Going to go with this simpler plan for now:
dist/  -> [delete]
docs/  -> docs-ui/
webapp/  -> ui/
.gitignore  -> .gitignore-ui
.travis.yml  -> .travis-ui.yml
README.md  -> README-UI.md

Everything else can stay as is, since there isn't a name conflict. We can then move/rename/edit things after the merge as appropriate.

The reason for deleting the dist directory as part of the merge is to reduce the size of the files being merged, given how much churn there is in that folder (particularly given bug 1145083). We can just regenerate it after merge - the history of those files isn't important.
(In reply to Ed Morley [:emorley] from comment #18) 
> Everything else can stay as is, since there isn't a name conflict. We can
> then move/rename/edit things after the merge as appropriate.

I'd be happy to contribute PRs for post-move/rename/edits later, if needed :)
Attached file ui-repo-merge.sh (obsolete) —
So with Mercurial, you can use the convert command along with a nice tidy filemap that contains "include", "exclude" and "rename" lines to rewrite history. With Git, unless you just want to move everything into a new subdirectory (ie subtree merge, or similarly simple changes), you have to resort to the filter-branch --index-filter + ls-files + sed + update-index combo.

I'm using index-filter instead of tree-filter, since it doesn't have to update the working directory, so is faster (still takes 15 minutes to run). Prune empty removes commits that no longer contain any changes (eg the grunt build commits that only touched dist/).

In the end I've gone for something a little more involved than the simpler renames in comment 18. Let me know if different names/structure is preferred:

  dist/  ->  [delete]
  docs/  ->  docs/ui/
  webapp/app/  ->  ui/
  webapp/test/  ->  tests/ui/
  webapp/config/  ->  tests/ui/config/
  webapp/logs/  ->  tests/ui/logs/
  webapp/scripts/web-server.js  ->  web-server.js
  webapp/scripts/  ->  tests/ui/scripts/
  webapp/.gitignore  ->  .gitignore-ui-webapp
  .gitignore  ->  .gitignore-ui
  .travis.yml  ->  .travis-ui.yml
  README.md  ->  README-UI.md

I've also gone for a merge rather than a rebase, which means there are two unrelated roots, the HEADs of which get merged. The intention of this is to avoid dumping 1800 commits on master HEAD, which is going to make it hard to see recent activity when using the git webui, sourcetree (in date order view) or using github compare links after deploying. Let me know if there are any downsides to this vs a rebase that I may not have considered.

For example script output (and so also the resultant paths of the UI files), see:
https://emorley.pastebin.mozilla.org/8833693

I should note this script won't work on OS X unless you have installed a recent gnu sed (since the native OS X sed doesn't support '\t').

After this merge is pushed, we'll obviously need to update quite a few files - eg: combine the various .gitignores/readmes, fix the paths for the directories in webapp/ that were moved, change the relative paths in web-server.js etc.
Attachment #8606578 - Flags: feedback?(wlachance)
Attachment #8606578 - Flags: feedback?(mdoglio)
Attachment #8606578 - Flags: feedback?(cdawson)
You can also browse the new ui repo layout here:
https://github.com/edmorley/treeherder-ui-tmp
Attachment #8606578 - Attachment mime type: application/x-shellscript → text/plain
Depends on: 1165697
Depends on: 1165702
Depends on: 1165709
Status: NEW → ASSIGNED
Depends on: 1165752
Comment on attachment 8606578 [details]
ui-repo-merge.sh

I tried to run it locally and I got this https://pastebin.mozilla.org/8833801
Are those merge conflicts expected?
Are you using the native OS X sed (which is severely deficient; comment 20)?
yeah, that was it. I tried with gnu sed and I didn't see any error
Attachment #8606578 - Flags: feedback?(mdoglio) → feedback+
Attached file ui-repo-merge.sh
To make this compatible with OS X's ed, I've replaced the instances of '\t' with a literal tab character.

The script has also been updated against the changes in bug 1165709, bug 1165702, and now also removes the webapp/app/resources/logs/ directory mentioned in comment 22.
Attachment #8606578 - Attachment is obsolete: true
Attachment #8606578 - Flags: feedback?(wlachance)
Attachment #8606578 - Flags: feedback?(cdawson)
Attachment #8606978 - Flags: feedback?(wlachance)
Attachment #8606978 - Flags: feedback?(cdawson)
Attachment #8606978 - Attachment mime type: application/x-shellscript → text/plain
Attachment #8594811 - Attachment description: Update links & puppet/Vagrant config for new repo name → Changes for treeherder-service rename
Comment on attachment 8606978 [details]
ui-repo-merge.sh

After doing the merge locally (which worked great, btw. :] ) I was browsing the directories.  The only thing that "feels" a little odd is to have a /ui folder and then a /treeherder/webapp/api folder.  

Perhaps this just needs to be separate due to the way we use Angular and Django.  Or could /ui live as a peer of /api?  I realize that puts things deeper in the folder structure, but when new people come in, it seems like that would be a logical place to find it.

I'm probably missing something as to how sacred the /webapp folder is to Django.  But thought I'd mention it.  If I'm off in the weeds, please ignore me.  :)

Otherwise, this is awesome.  Thanks for doing this hard work, Ed!!
Attachment #8606978 - Flags: feedback?(cdawson) → feedback+
(In reply to Cameron Dawson [:camd] from comment #27)
> After doing the merge locally (which worked great, btw. :] ) I was browsing
> the directories.  The only thing that "feels" a little odd is to have a /ui
> folder and then a /treeherder/webapp/api folder.  
> 
> Perhaps this just needs to be separate due to the way we use Angular and
> Django.  Or could /ui live as a peer of /api?  I realize that puts things
> deeper in the folder structure, but when new people come in, it seems like
> that would be a logical place to find it.

By 'peer' of treeherder/webapp/api/, do you mean treeherder/webapp/ui/ ?

I kind of see treeherder/webapp/ as really being the API (perhaps treeherder/api/ would be a better name for it?), so I'm not sure if the ui should go inside there.

However I wonder if the ui should go in treeherder/ui/ (rather than in the repo root), seeing as the client is in treeherder/client/ too?

The treeherder directory really seems like a src directory (again perhaps src/ would have been a better name for it, though I guess that's against python package naming patterns) so having ui+client+service source inside there (and docs/tests outside it and in their respective directories) might make sense?

This would give:
treeherder/client/    
treeherder/embed/     
treeherder/etl/       
treeherder/events/    
treeherder/log_parser/
treeherder/model/     
treeherder/settings/  
treeherder/ui/    
treeherder/webapp/    
treeherder/workers/   

And then if we later renamed treeherder/webapp/ to treeherder/api/ it would make even more sense.
(In reply to Ed Morley [:emorley] from comment #28)
> And then if we later renamed treeherder/webapp/ to treeherder/api/ it would make even more sense.

++ and the directory naming above is looking nice.
Comment on attachment 8606978 [details]
ui-repo-merge.sh

Approve of the general direction here.
Attachment #8606978 - Flags: feedback?(wlachance) → feedback+
Depends on: 1166426
When do we feel is a good time to do this then? Is tomorrow ok?
We'll just need to stop committing to the UI repo from the time I start, and won't be able to push any UI changes (or backouts) to prod until the apache changes have been made in the puppet repo after I've landed everything.
sgtm :)

I think whimboo was planning on getting running locally tomorrow with treeherder for hacking on or using the client, so cc'ing him for visibility.
Merge of treeherder-ui into treeherder complete:
https://github.com/mozilla/treeherder/commit/347592a10e29aa5bc7ea525b64c45e0e8dc1c87a

I went with the ui in 'ui/' (ie same as attached script) rather than my alternative in comment 28, after discussing with Mauro yesterday (tl;dr once we switch to the UI being served by Django, the structure will change a bit again, so not worth trying to guess that now, and it doesn't make sense to have the UI inside the python module until it's actually served by Django).

Permissions to the Github repos have been set to read only until the transition is finished.
With this PR:
* .gitignore works as expected
* web-server.js works
* vagrant works
* The documentation is much better (but we could do with combining some of the pages later on). The UI pages currently appear under 'ui/' on the service readthedocs.
* The UI test configs/scripts _should_ work, but they're still not hooked up to Travis (making a travis file that works with both python and nodejs tests will need some thought).
* grunt build works (puts the output in dist/ as before)

Things left to do now:
* Check this PR in once reviewed
* Check in the output of grunt build
* Push to stage
* Update the stage apache configs (bug 1165752)
* Presuming that is fine, push to prod
* Update the prod apache configs (bug 1165752)

To do later:
* Get the UI tests running as part of the travis run
* Combine the service and UI doc installation pages etc
* Update the wiki page
Attachment #8608081 - Flags: review?(mdoglio)
Oh and:
* Update the UI repo so there's just a README or similar in the root explaining the repo move
* Restore write permissions to the treeherder github team to the treeherder repos
* Once we're happy with prod, remove the old ui src repo
Depends on: 1145083
Comment on attachment 8608081 [details] [review]
Changes required post UI repo merge

My eyes didn't catch anything suspicious
Attachment #8608081 - Flags: review?(mdoglio) → review+
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9210d9bf2763c6aaaa908cddb7eaaf3dddd10b1a
Bug 1056877 - Merge .gitignore and .gitignore-ui

https://github.com/mozilla/treeherder/commit/b4c7023ccc9171534a57875cf9ff5befe65a88b2
Bug 1056877 - Remove superfluous docs/ui/ Sphinx build files

Since the UI docs will be built by the docs directory Makefile/conf.py.

https://github.com/mozilla/treeherder/commit/de9b1cff0244692abdf7b5cd888ee5e9978836b7
Bug 1056877 - Add the UI docs pages to the main table of contents

The UI docs are now being built at the same time as the main service
docs. Until we combine them, let's at least make sure the UI parts are
still discoverable, by adding them to the main table of contents.

https://github.com/mozilla/treeherder/commit/da294ba7611d2b31cf9a5941095f597a79057ea7
Bug 1056877 - Stop using/referring to the separate treeherder-ui repo

Since it has now been merged into this one.

https://github.com/mozilla/treeherder/commit/2efb703f165366e2068134a13375d7a1a23b7105
Bug 1056877 - Update UI path references after directory moves

As part of merging the UI repo into this one, the following directory
moves were performed:
  webapp/app/                   ->  ui/
  webapp/test/                  ->  tests/ui/
  webapp/config/                ->  tests/ui/config/
  webapp/scripts/               ->  tests/ui/scripts/
  webapp/scripts/web-server.js  ->  web-server.js
Output of grunt build now checked into the main repo:
https://github.com/mozilla/treeherder/commit/797eb1431eb9788c481389bc895656fe802a0341
(using the update packages after bug 1145083 - everyone needs to make sure they've run |npm install| in the treeherder repo after that landed).
> * Check this PR in once reviewed
> * Check in the output of grunt build
> * Push to stage
> * Update the stage apache configs (bug 1165752)
> * Presuming that is fine, push to prod
> * Update the prod apache configs (bug 1165752)

This is all done now :-)

> * Update the wiki page

Done:
https://wiki.mozilla.org/index.php?title=Auto-tools%2FProjects%2FTreeherder&diff=1075570&oldid=1074300

Also:

* Restore write access to the treeherder repos

Done

* Rename the old treeherder-ui master to old-master and add a readme to the repo root under a new master branch

Done:
https://github.com/mozilla/treeherder-ui/commit/9bf0b6e39d61c2b0b374b16ca3b9dd7960dfa400

> * Get the UI tests running as part of the travis run
> * Combine the service and UI doc installation pages etc

I'll file bugs for these.

+ I'll do something about the PRs that got closed when master was renamed to old-master:
https://github.com/mozilla/treeherder-ui/pull/353
https://github.com/mozilla/treeherder-ui/pull/524
https://github.com/mozilla/treeherder-ui/pull/501
https://github.com/mozilla/treeherder-ui/pull/519
https://github.com/mozilla/treeherder-ui/pull/523
Depends on: 1166901
Blocks: 1166905
Use this script when in the treeherder-ui repo and an old PR is checked out, to export a patch with paths rewritten, that should apply against the main treeherder repo.
Post to dev.tree-management about the repo merge:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/baZmYme4lkc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1166938
I've renamed the ui and client github repos, such that there is a '-deprecated' appended to the name now (Github puts redirects in place, so old links will work - it will just make the EOL nature of them clearer on Google results etc).

I've also updated the URLs/descriptions for treeherder-client on pypi, plus the docker hub description (https://registry.hub.docker.com/u/treeherder/treeherder-service/) - though it's not possible to change the URLs on docker hub without deleting and starting over, so I'll save that for the future.
Blocks: 1170706
Component: Treeherder: Docs & Development → TreeHerder
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: