Skip to content
Snippets Groups Projects

Cleanup Ci::Commit, Ci::Build and CommitStatus views

Merged Kamil Trzciński requested to merge fix-commit-status-rendering into master

This MR tries to do first sweep of cleanups to Ci::Commit and Ci::Build objects removing all view-related functions and fixing the API from other side.

Fixes: https://gitlab.com/gitlab-org/gitlab-ce/issues/6046

/cc @grzesiek @DouweM

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Maintainer

    This is based on permission changes.

  • Kamil Trzciński Added 65 commits:

    Added 65 commits:

    • cc8fce78...7383453b - 43 commits from branch master
    • 3ba9e6fa - Make the CI permission model simpler
    • cc6cad6d - Add CI setting: allow_guest_to_access_builds
    • 58831775 - Clean Ci::ApplicationController from unused permission related code
    • ceae5c4a - Fix build errors
    • 8ad676dc - Properly handle commit status permissions (for a build)
    • 0f2394a2 - Simplify abilities
    • 126dc66c - Expose allow_guest_to_access_builds in GitLab API
    • 64640f89 - Rename allow_guest_to_access_builds to public_builds
    • 9a70df95 - Add behaviour tests for build permissions
    • abb5d243 - Update ability model after comments
    • 19c44414 - Use delete instead of assignment operator when filtering build abilities
    • 01043bcf - Remove current_user && when can? is used
    • c7bebe6e - Fix commit status tests
    • 53045c53 - Introduce API for serving the artifacts archive
    • afc5ee3c - Get rid of controller related logic in models
    • c30b5929 - Return to the same page when retrying or canceling the build
    • 2f4e07de - Force to refresh build view when pending->running
    • 4ea9ffb8 - Remove unused Ci::Commit.last_build
    • d260e23d - Remove unused Ci::Commit.ordered
    • 31bba51e - Removed unused Ci::Commit.update_committed
    • 5740be0e - Remove unused ci/commits/_commit view
    • b70a15c7 - Try to remove as much as build related logic from Ci::Commit
  • Kamil Trzciński mentioned in merge request !2560 (merged)

    mentioned in merge request !2560 (merged)

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 18 18 = link_to project.name_with_namespace, admin_namespace_project_path(project.namespace, project), class: "monospace"
    19 19
    20 20 %td
    21 = link_to build.short_sha, namespace_project_commit_path(project.namespace, project, build.sha), class: "monospace"
    21 = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace"
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 375 376 expose :name
    376 377 end
    377 378
    379 class BuildArtifactFile < Grape::Entity
    380 expose :filename, :size
    381 end
    382
    378 383 class Build < Grape::Entity
    379 384 expose :id, :status, :stage, :name, :ref, :tag, :coverage
    380 385 expose :created_at, :started_at, :finished_at
    381 386 expose :user, with: User
    382 # TODO: download_url in Ci:Build model is an GitLab Web Interface URL, not API URL. We should think on some API
    383 # for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255)
    384 expose :download_url do |repo_obj, options|
    • Should we keep this for backward compatiblility?

    • Author Maintainer

      Previously it were not working as expected. We were returning the URL to controller that is outside of API. We could potentially return the URL download using GitLab API, but this doesn't make sense either since it requires special authentication.

    • Author Maintainer

      Nope. This was broken before, because it referred to a controller action. Right now we add a dedicated build artifacts API.

    • Author Maintainer

      No, it doesn't make sense. It was broken in the past. The URL here were to GitLab controller serving the content. Right now we have dedicated API to download artifacts for a build.

  • @ayufan Huge MR is huge and hard to review :) But I didn't see anything that struck me as unexpected.

    @rspeicher Can you have a look-over as well?

  • Author Maintainer

    @DouweM It's not that big. I need to rebase it it will be then a houndred lines long :)

  • Kamil Trzciński Added 206 commits:

    Added 206 commits:

    • b70a15c7...7cc4b739 - 197 commits from branch master
    • c0c964e0 - Introduce API for serving the artifacts archive
    • 52f23f3a - Get rid of controller related logic in models
    • 73666062 - Return to the same page when retrying or canceling the build
    • 2afd4350 - Force to refresh build view when pending->running
    • f49f5122 - Remove unused Ci::Commit.last_build
    • fdb63046 - Remove unused Ci::Commit.ordered
    • 1f46691d - Removed unused Ci::Commit.update_committed
    • ed4e11e5 - Remove unused ci/commits/_commit view
    • 4471eec7 - Try to remove as much as build related logic from Ci::Commit
  • Kamil Trzciński Added 1 commit:

    Added 1 commit:

    • 77499d87 - Make the CiBuild JS easier to understand
  • This MR tries to do first sweep of cleanups to Ci::Commit and Ci::Build objects removing all view-related functions and fixing the API from other side.

    It introduces a missing download artifacts API in context of GitLab API.

    It will also resolve: #6046 (closed).

    This sounds like three separate MRs :)

  • Author Maintainer

    @rspeicher Thanks I'll split this :)

  • Kamil Trzciński Added 126 commits:

    Added 126 commits:

    • 77499d87...c21c8825 - 117 commits from branch master
    • 8cb838de - Get rid of controller related logic in models
    • 39b3524f - Return to the same page when retrying or canceling the build
    • d45dd972 - Force to refresh build view when pending->running
    • a3c167c9 - Remove unused Ci::Commit.last_build
    • a62e5291 - Remove unused Ci::Commit.ordered
    • 32c4ef4c - Removed unused Ci::Commit.update_committed
    • d0daeaf4 - Remove unused ci/commits/_commit view
    • 0e80b41c - Try to remove as much as build related logic from Ci::Commit
    • c0816ee1 - Make the CiBuild JS easier to understand
  • Kamil Trzciński Title changed from [WIP] Cleanup Ci::Commit, Ci::Build and CommitStatus views to Cleanup Ci::Commit, Ci::Build and CommitStatus views

    Title changed from [WIP] Cleanup Ci::Commit, Ci::Build and CommitStatus views to Cleanup Ci::Commit, Ci::Build and CommitStatus views

  • Kamil Trzciński Added ~19173 label

    Added ~19173 label

  • Reassigned to @ayufan

  • Kamil Trzciński Added 1 commit:

    Added 1 commit:

  • Kamil Trzciński Added 47 commits:

    Added 47 commits:

    • 82db8bbd...1817b766 - 37 commits from branch master
    • 0d698492 - Get rid of controller related logic in models
    • 3c361040 - Return to the same page when retrying or canceling the build
    • 2a7879ba - Force to refresh build view when pending->running
    • d51e28ad - Remove unused Ci::Commit.last_build
    • 6306b950 - Remove unused Ci::Commit.ordered
    • 1cb81de2 - Removed unused Ci::Commit.update_committed
    • 3f53900f - Remove unused ci/commits/_commit view
    • 21790657 - Try to remove as much as build related logic from Ci::Commit
    • f9696e44 - Make the CiBuild JS easier to understand
    • 86dcc917 - Fix broken merge. The BuildArtifactsFile is introduced by https://gitlab.com/git…
  • Kamil Trzciński Added 1 commit:

    Added 1 commit:

    • 89057f79 - Bring back the before_sha
  • mentioned in issue #4263 (closed)

  • Kamil Trzciński Added 1 commit:

    Added 1 commit:

    • ed23d4a3 - Fix specs
  • Kamil Trzciński Added 50 commits:

    Added 50 commits:

    • ed23d4a3...96158aa6 - 38 commits from branch master
    • f0293398 - Get rid of controller related logic in models
    • 36027e62 - Return to the same page when retrying or canceling the build
    • b42a06f2 - Force to refresh build view when pending->running
    • caabb882 - Remove unused Ci::Commit.last_build
    • 11486981 - Remove unused Ci::Commit.ordered
    • 6f844eeb - Removed unused Ci::Commit.update_committed
    • 87a201ad - Remove unused ci/commits/_commit view
    • 3f124260 - Try to remove as much as build related logic from Ci::Commit
    • 78e646b6 - Make the CiBuild JS easier to understand
    • 1a7724f7 - Fix broken merge. The BuildArtifactsFile is introduced by https://gitlab.com/git…
    • c8c8d898 - Bring back the before_sha
    • 1aca3eb5 - Fix specs
  • Author Maintainer

    This MR basically moves the commit_statuses/_commit_status into two separate views one for builds and one for generic status and removes all view related methods that did end-up in models.

  • Author Maintainer

    @rspeicher This is mostly the reordering. Do you find something that I should fix here?

  • Kamil Trzciński Added 97 commits:

    Added 97 commits:

    • 1aca3eb5...2cc9a42c - 85 commits from branch master
    • c6336206 - Get rid of controller related logic in models
    • 95179532 - Return to the same page when retrying or canceling the build
    • 9b4dd5f2 - Force to refresh build view when pending->running
    • ed4e0a27 - Remove unused Ci::Commit.last_build
    • 6975d021 - Remove unused Ci::Commit.ordered
    • 1b128de0 - Removed unused Ci::Commit.update_committed
    • 371a26dc - Remove unused ci/commits/_commit view
    • 7132e42c - Try to remove as much as build related logic from Ci::Commit
    • b71286f1 - Make the CiBuild JS easier to understand
    • 18230fa8 - Fix broken merge. The BuildArtifactsFile is introduced by https://gitlab.com/git…
    • 85c9cd83 - Bring back the before_sha
    • 32c9db59 - Fix specs
  • mentioned in issue #13559 (closed)

  • Kamil Trzciński Added 739 commits:

    Added 739 commits:

    • 32c9db59...5956ddd8 - 738 commits from branch master
    • 5920b90c - Cleanup CiCommit and CiBuild
  • Kamil Trzciński Added 27 commits:

    Added 27 commits:

    • 5920b90c...d9042e8b - 25 commits from branch master
    • 6638a493 - Merge remote-tracking branch 'origin/master' into fix-commit-status-rendering
    • b095cb84 - Merge remote-tracking branch 'origin/master' into fix-commit-status-rendering
  • Kamil Trzciński Added 1 commit:

    Added 1 commit:

    • 51caa64a - Fix commit_spec: invalid validation
  • Author Maintainer

    @DouweM Please take a look.

  • @rspeicher Can you review this?

  • 309 309 project.valid_runners_token? token
    310 310 end
    311 311
    312 def target_url
    • Assuming no more of these *_url helpers exist in the model, can we remove include Gitlab::Application.routes.url_helpers from the top?

  • 1 %tr.build
    2 %td.status
    3 - if can?(current_user, :read_build, build)
    4 = link_to namespace_project_build_url(build.project.namespace, build.project, build), class: "ci-status ci-#{build.status}" do
    5 = ci_icon_for_status(build.status)
    6 = build.status
    7 - else
    8 = ci_status_with_icon(build.status)
  • 401 401 expose :id, :status, :stage, :name, :ref, :tag, :coverage
    402 402 expose :created_at, :started_at, :finished_at
    403 403 expose :user, with: User
    404 # TODO: download_url in Ci:Build model is an GitLab Web Interface URL, not API URL. We should think on some API
    405 # for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255)
    406 expose :download_url do |repo_obj, options|
  • @DouweM @ayufan A couple questions/issues.

  • Author Maintainer

    @rspeicher I'll fix all of this :)

  • Reassigned to @DouweM

  • Kamil Trzciński Added 222 commits:

    Added 222 commits:

    • 51caa64a...37ba5a12 - 219 commits from branch master
    • 06722589 - Cleanup CiCommit and CiBuild
    • f32e28f6 - Fix commit_spec: invalid validation
    • 16592e2b - Fix review comments
  • Reassigned to @rspeicher

  • @rspeicher Can you do it please? :)

  • Robert Speicher mentioned in commit c4b35a62

    mentioned in commit c4b35a62

  • Robert Speicher Status changed to merged

    Status changed to merged

  • Robert Speicher Milestone changed to 8.6

    Milestone changed to 8.6

  • mentioned in issue #14281 (closed)

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading