Cleanup Ci::Commit, Ci::Build and CommitStatus views
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.
Merge request reports
Activity
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
Toggle commit list- cc8fce78...7383453b - 43 commits from branch
mentioned in merge request !2560 (merged)
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" 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| @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?
@DouweM It's not that big. I need to rebase it it will be then a houndred lines long :)
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
Toggle commit list- b70a15c7...7cc4b739 - 197 commits from branch
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 :)
@rspeicher Thanks I'll split this :)
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
Toggle commit list- 77499d87...c21c8825 - 117 commits from branch
Reassigned to @ayufan
Added 1 commit:
- 82db8bbd - Fix broken merge. The BuildArtifactsFile is introduced by https://gitlab.com/git…
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…
Toggle commit list- 82db8bbd...1817b766 - 37 commits from branch
mentioned in issue #4263 (closed)
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
Toggle commit list- ed23d4a3...96158aa6 - 38 commits from branch
@rspeicher This is mostly the reordering. Do you find something that I should fix here?
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
Toggle commit list- 1aca3eb5...2cc9a42c - 85 commits from branch
mentioned in issue #13559 (closed)
Added 739 commits:
- 32c9db59...5956ddd8 - 738 commits from branch
master
- 5920b90c - Cleanup CiCommit and CiBuild
- 32c9db59...5956ddd8 - 738 commits from branch
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
- 5920b90c...d9042e8b - 25 commits from branch
@DouweM Please take a look.
@rspeicher Can you review this?
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) You spotted an issue @rspeicher, too bad it wasn't addressed right away! I created a new issue for that: https://gitlab.com/gitlab-org/gitlab-ce/issues/14281
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| Is it going to be a problem removing this from the API without notice? See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3103/diffs#cc752c66542cbeeb21ca7a57c430836b865580c9_144_144 where I was recently not allowed to remove something from the API that hadn't been used in years.
@rspeicher I have problem with what should be here. Since this is a fairly new API it should be OK to remove it. We should only add a proper CHANGELOG and notify in blog post that such change did happen.
Edited by Kamil Trzciński
@rspeicher I'll fix all of this :)
@rspeicher @DouweM Fixed!
Reassigned to @DouweM
Reassigned to @rspeicher
@rspeicher Can you do it please? :)
mentioned in commit c4b35a62
Milestone changed to 8.6
mentioned in merge request gitlab-com/www-gitlab-com!1633 (merged)
mentioned in issue #14281 (closed)