Skip to content
GitLab
Next

Draft: [PoC] Pages merge request preview

Closed Kassio Borges requested to merge kassio/spike-pages-review-apps into master

What does this MR do and why?

⚠ NOT TO BE MERGED ⚠

Idea

When a pages build is a MR build, create an option for the users to deploy the build to a temporary review app on a random URL. This will enable users to review changes on Pages sites before deploying to production.

When triggered, the idea is to use Projects::UpdatePagesService, which creates a PagesDeployment, to also create a Pages::MergeRequestPreview (new model/table) which associates the deploy to a MergeRequest and the random-ish slug. This way, when the looking for that slug, in Gitlab::Pages::VirtualHostFinder, we can lookup for that specific PagesDeployment to serve via GitLab Pages.

Update 2023-06-08
  • The current strategy requires the users to include a pages:preview job in their gitlab-ci.yml. This way if the users wants to change how to build the previews they can.
  • We're only keeping one build/deploy per MR. This way, if the MR is updated, the slug/url will be the same for all the builds.
  • After reading more the history of the requested feature, I'm leaning more to rename the feature to Pages merge request preview, which I think describes better the feature and avoid confusion with review-apps (which AFAIK is a totally different feature internally)
Update 2023-06-09
  • Update the associations for better description of the data-flow
  • Improved the Pages::MergeRequestPreview creation/updating. The new strategy made it easy to ensure to use the same URL/slug during the MR lifetime. Now it'll also be possible to use a real random string in the URL.
Update 2023-06-12
  • The pages merge request preview is being created on each build of a pages:preview job in a Merge Request. When the merge request is merged or closed the PagesDeployment is deleted.
    • This was done using the Gitlab Event Store, by the introduction of MergeRequest::MergedEvent and MergeRequest::ClosedEvent.
  • When a new production version of a pages site is deployed we don't delete pages merge request previews.
Update 2023-06-13
  • Added $CI_PAGES_MERGE_REQUEST_PREVIEW_URL to expose the merge_request preview URL during the build:
Screenshot_2023-06-13_at_18.54.46
Update 2023-06-15

To avoid possible traffic hijack, where a user could create a project with the path of a pages preview, (Thanks @vshushlin for bringing this to my attention), I changed the format of the preview slug to use * as separtor. So now, the slug would be like: preview*<23 chars of the project path>*<random hash>. * is a valid URL char, but it's not accepted as a project slug/path.

TODO

  • When the pages deploy is a MR, create a random slug for the build
    - This is saved in the Pages::MergeRequestReviewApp
  • Make it possible to lookup pages by the random slug created
  • When MR is merged or closed, delete all the associated deploys and previews
  • Currently, when a new version of a pages site is deployed, we scheduled to delete the old deploys for that project. We must do that in a way that it doesn't delete preview deploys.
  • Figure a way to expose the created URL to the users: maybe inject an environment variable in the build with the preview URL for that MR and add it somewhere in the UI. I decided to not do this on the PoC (more info)
  • Add the pages merge request preview URL in the UI to improve accessibility

Questions

  1. Some projects already have Review Apps which gives access to their pages, what's the difference here?
    • When a project pipeline builds pages aside of the main project code, currently, the review app also builds the main project. The goal here, is to enable users to build only their pages in a review app. In other words, build only the :pages job as a review app.
  2. When delete review app pages deploys?
    • On a new MR build?
    • When the MR is closed/Merged?
  3. A new table to associate a PagesDeployment with the MergeRequest, and give it a slug was created. Would add the merge requests association and slug directly to PagesDeployment make it easier to handler the deletion of outdated PagesDeployments? The currently NOT EXISTS query seems a bit complex and using everything in the same table would be a bit easier (PagesDeployments.where(merge_request_id: null)...)
Update 2023-06-13
  1. Currently we're only deleting the PageDeployment object from the database when the MR is merged or closed. I was wondering if we should, after a while, also delete the Pages::MergeRequestPreview record? The reason for that is: currently Pages::MergeRequestPreview generates random slug, keeping old records would limit the number of slugs we'd be able to serve.
    • My proposal would be to delete these records and document that the slug/URL is only singular by MR while it's not closed/merged. If a closed MR is re-open, it'd have a new pages preview slug.

Related to:

Screenshots or screen recordings

Screen_Recording_2023-06-15_at_09.16.00

How to set up and validate locally

Example of the build job:

pages:preview:
  stage: deploy
  script:
  - 'echo "Accessible via: $CI_PAGES_MERGE_REQUEST_PREVIEW_URL"'
  artifacts:
    paths:
    - public
  only:
  - merge_requests

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Kassio Borges

Merge request reports