1104718 - remove rewrite hacks from new balrog ui install

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

We had to use these apache rewrite hacks to make the new Balrog UI work, because Angular doesn't like to run in a subdirectory:
    # Host the Angular app UI in a subdirectory but...
    Alias /ui /home/vagrant/project/ui/dist/

    # ...Angular doesn't work properly in a subdirectory, so we need to
    # rewrite any endpoints it actually uses to /ui, otherwise they 404.
    RewriteEngine On
    RewriteRule ^/css/app.css /ui/css/app.css [PT]
    RewriteRule ^/js/app.js /ui/js/app.js [PT]
    RewriteRule ^/favicon.ico /ui/favicon.ico [PT]
    RewriteRule ^/img(.*) /ui/img$1 [PT]
    RewriteRule ^/fonts(.*) /ui/fonts$1 [PT]

    WSGIScriptAlias / /home/vagrant/project/admin.wsgi


It works pretty much fine, but it breaks refreshing, because pages like /ui/rules are rewritten to /rules - which doesn't exist.

We need to change the install so that the Angular app is hosted at /. This means either moving the api to a subdirectory or creating a new domain for it. Either way, client scripts will need to be updated and we'll need a grace period while we do that.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: use /api in buildbot configs

It looks to me like the simplest way forward here is to start using the /api endpoints in all of our submission scripts. AFAICT, the endpoints that we rely use the same code in both versions:
app.add_url_rule("/api/releases/<release>", view_func=SingleReleaseView.as_view("api_releases_revision"))
app.add_url_rule('/releases/<release>', view_func=SingleReleaseView.as_view('release'))

app.add_url_rule("/api/releases/<release>/builds/<platform>/<locale>", view_func=SingleLocaleView.as_view("api_single_locale"))
app.add_url_rule('/releases/<release>/builds/<platform>/<locale>', view_func=SingleLocaleView.as_view('single_locale'))

app.add_url_rule("/api/rules/<rule_id>", view_func=SingleRuleView.as_view("api_rule"))
app.add_url_rule('/rules/<rule_id>', view_func=SingleRuleView.as_view('setrule'))

Once this, and the two other incoming patches land, we should be able to rip out all of the non-/api endpoints.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: use /api in mozharness

Comment 3

[[e-mail] (:bhearsum)]

Attachment: use /api in tools

Comment 4

[[e-mail] (:bhearsum)]

Attachment: rip out the old balrog ui

This patch rips out the old Balrog UI as well as adjust the testing environments to put the Angular app at / (and the api at /api). The details are a bit more complex...because nothing is ever simple.

The main complexity is around how the API is served. The WSGI app itself doesn't know anything about the "/api" prefix. I did this primarily because I couldn't get the Apache config working any other way, but it also leaves us the option of easily moving it to another domain later (eg, aus4-api.mozilla.org, where we wouln't want "/api" in the URLs). Both the apache config and admin.py have a bit of code around this, and there's a lot of 's,/api//,g' sprinkled elsewhere to remove the app's knowledge of it.

Most of the rest is just dead code removal. I think it's obvious for the most part except maybe the ReleaseHistoryView hunk that removes pagination (which is handled on the client side now).

I also changed balrog-server.py's default port to 7000 to avoid conflicting with lineman's default of 9000.

This patch omits all of the templates and static file removals. You can view the full (massive) version at https://github.com/bhearsum/balrog/compare/kill-old-ui?expand=1 if you want.

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8558578 [details] [diff] [review]
use /api in tools

Sheriffs has asked that I trigger a nightly build to test these changes. I merged the mozharness patch to production and triggered a new Flame nightly on mozilla-central to verify this.

Comment 6

[Rail Aliiev [:rail]]

In production: https://hg.mozilla.org/build/buildbot-configs/rev/1f76b8e0cb8a

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #5)
> Comment on attachment 8558578 [details] [diff] [review]
> use /api in tools
> 
> Sheriffs has asked that I trigger a nightly build to test these changes. I
> merged the mozharness patch to production and triggered a new Flame nightly
> on mozilla-central to verify this.

I ended up triggering a Linux nightly too. Both of these succeeding using the /api endpoints \o/.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8558745 [details] [diff] [review]
rip out the old balrog ui

Review of attachment 8558745 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a few improvements on landing.

::: admin.py
@@ +14,5 @@
> +        self.wrap_app = wrap_app
> +
> +    def __call__(self, environ, start_response):
> +        environ["PATH_INFO"] = environ["PATH_INFO"].lstrip("/api")
> +        print environ

Oopsie.

@@ +72,5 @@
>      def auth(environ, username, password):
>          return username == password
> +    # The deployed version of Balrog (and the Vagrant environment) both put the
> +    # admin API endpoints behind "/api". We should do the same here for
> +    # consistency.

It took me a while to wrap my head around this, so it would be good to expand on this for posterity.

::: auslib/admin/base.py
@@ +39,5 @@
>  
>  Compress(app)
>  
>  # Endpoints required for the Balrog 2.0 UI.
> +app.add_url_rule("/csrf_token", view_func=CSRFView.as_view("csrf"))

A comment here explaining why these are not at /api would be good. eg someone new to Balrog sees a request to /api/<foo> then there is no route for that.

@@ +49,5 @@
> +app.add_url_rule("/rules", view_func=RulesAPIView.as_view("rules"))
> +app.add_url_rule("/rules/<rule_id>", view_func=SingleRuleView.as_view("rule"))
> +app.add_url_rule("/rules/<rule_id>/revisions", view_func=RuleHistoryAPIView.as_view("rules_revisions"))
> +app.add_url_rule("/releases", view_func=ReleasesAPIView.as_view("releases"))
> +app.add_url_rule("/releases/<release>", view_func=SingleReleaseView.as_view("releases_revision"))

I know this is just an arbitrary string, but 'releases_revision' seems wrong, maybe just 'release'.

::: auslib/db.py
@@ +1025,1 @@
>          # TODO: Remove these old endpoints when old ui dies.

Lets zapp'em too then, and double check if anyone has /api/... perms in prod.

::: puppet/files/etc/httpd/conf.d/balrog.conf
@@ +30,3 @@
>  
> +    # Rewrite virtual paths in the angular app to the index page
> +    # so that refreshes/linking works.

s#\.#, while leaving js/css/etc requests alone.# ?

Comment 9

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #8)
> Comment on attachment 8558745 [details] [diff] [review]
> rip out the old balrog ui
> 
> Review of attachment 8558745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a few improvements on landing.

All of this should be addressed in https://github.com/bhearsum/balrog/commit/f6e812a8c505630ad80131c019db43b676fc7416.

Comment 10

[[github robot]]

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

https://github.com/mozilla/balrog/commit/86307283c8aab6aeb7b1abb7128b2f510c138b8c
bug 1104718/1104719: kill and rip out the old balrog ui. r=nthomas

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8558745 [details] [diff] [review]
rip out the old balrog ui

This push is happening any minute now, I've pushed patch.

Comment 12

[[e-mail] (:bhearsum)]

I broke a few tests when landing here, but nothing crucial. I forgot to retest after merging :(. I'll fix them up ASAP.

Comment 13

[[e-mail] (:bhearsum)]

Attachment: fix tests after merge

Silly me forgot to rerun tests after merging before pushing. My main patch here removed /api from the internal routes of the app, so the tests need to respect that and just use "/releases", etc.

Comment 14

[[github robot]]

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

https://github.com/mozilla/balrog/commit/88617f7e1770bae42b5b33ec2215ba538f814bfc
bug 1104718: remove rewrite hacks from new balrog ui install - fix tests that were missed in merge. r=rail

Comment 15

[[e-mail] (:bhearsum)]

Alright, this is landed and appears to be working fine. My latest patch fixed the errors (which were test problems, not code problems), so this is all done \o/.