Skip to content

Add better handling for url and path helpers

Jeremy Jackson requested to merge jj-resolve-routes-for-urls into master

This improves the logic used to generate urls and utilizes the native rails url logic for host information.

This is required for us to be able to use the experiment redirects within README.md files.

So, we're using this inside the gitlab README.md files for new projects, and we were using relative paths, but gitlab rewrites all relative paths inside README.md files to be relative to the repo that it belongs to, and so our relative paths are relative to the repo, and not to gitlab, which we don't want. This is actually pretty nice in general terms, but requires us to render a full url with protocol and domain.

We were generating /-/experiment/example:abc123 type urls, but these are being rewritten to /jejacks0n/project/master/-/experiment/example:abc123. Because of this, we want to generate a full canonical url, and to do this, we have to use the host that's specified in the routing layer. Since routing within engines is a bit of a complex thing, I added a more comprehensive route strategy, which inherits the default_url_options from the application it's mounting within. Doing it this way allows us to use proper *_url helpers, and makes them behave as expected.

There's now a "resources" route for experiments (only show for now), and this MR enhances the base Gitlab::Experiment class with ActiveModel::Model when that's needed, as you can't use the url helpers without having some of that logic mixed in.

This now allows us to do things, and expand on the routing concepts like:

ex = experiment(:example)
experiment_engine.experiment_path(ex) # => "/-/experiment/example:abc123"

and this also leaves the redirect url helpers intact:

experiment_redirect_url(ex, url: 'https://example.com') # => "https://gitlab.com/-/experiment/example:abc123?https://example.com"

Default Configuration Change:

We no longer mount the middleware/engine by default, for security safety reasons.

Edited by Jeremy Jackson

Merge request reports