WIP Compare codeclimate artifacts on merge request page
What does this MR do?
Show how code quality changes by the merge request widget if project has codeclimate job in .gitlab-ci.yml.
Are there points in the code the reviewer needs to double check?
JS code
Why was this MR needed?
Show how code quality changes by the merge request allow us and other GitLab users to increase software quality they develop.
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated API support added- Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
added 1 commit
- 2f0624d4 - Show introduced/resolved code quality issues on mr widget
mentioned in issue #4044 (closed)
added 1 commit
- e1db5acc - Remove garbage code for mr code quality widget
This solution is a lot simpler than I originally anticipated
Just some notes- glad to jump in and fix it myself!
- With Vue.js we no longer use
$.ajax, we now usevue-resource - All calls to the server should be made using the Service component of each app and in the main smart component, more details about this architecture here: https://docs.gitlab.com/ce/development/fe_guide/vue.html#vue-architecture
In this case we would need to the the following:
- Add
loadUrlmethod to the MR service: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js - Make this call in the main smart component: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
- Provide the data as a prop to the new component.
@dzaporozhets let me know if you need me to do it
- With Vue.js we no longer use
assigned to @filipa
@dimitrieh lets brainstorm possible UI/UX for this feature. I started with a primitive thing here https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11379#note_29783613 but we probably should make it into one liner with some possibility to expand info.
added 1 commit
- 98d1dd0e - Refactor merge request code quality component to match frontend documentation
@dzaporozhets really cool, this reminds me of an old design i once made for https://gitlab.com/gitlab-org/gitlab-ce/issues/25424#note_21800473 example image mockup
So my question here is, does this relay information only originating from failed CI jobs or are there other sources?
Also i like the way this displays the information, however i wonder about when this information is needed to be displayed. It can take up a lot of vertical space. In that sense it may make sense to hide it behind an expandable anchor:
Let me know what you think, I'll keep close watch of this mr
Edited by Dimitrie Hoekstra-
does this relay information only originating from failed CI jobs or are there other sources?
@dimitrieh no information is either positive like:
Code quality improvedor negative likecode quality decreased. However, CI is green because job is always positive (completed), similar to what we have now with test coverageEdited by username-removed-444 -
@dzaporozhets what about the following design?
expanded:
some thoughts:
- better symbols for good and bad?
-
@dimitrieh looks great!
@filipa can we make it look like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11379#note_29931113 ?
-
@dimitrieh regarding the icons, do we always show a green one if there is at least one improved point?
-
note: expanded area builds on @hazelyang idea of https://gitlab.com/gitlab-org/gitlab-ee/issues/1510 :)
-
note: from conversation with @filipa
If there are new issues there is a problem, only good situation is when all are resolved
so the icons will be either checkmark green or cross red
Edited by Dimitrie Hoekstra -
After talking with @dimitrieh:
- Show green icon when all issues are resolved
- Show red icon if there is any non resolved issue
-
@dimitrieh @filipa when code quality does not change or improved == green icon. If degraded == red icon.
added 273 commits
-
53264fab...4e410a6e - 272 commits from branch
master - 9c6a4be8 - Merge branch 'master' into dz-codeclimate-compare
-
53264fab...4e410a6e - 272 commits from branch
-
From slack:
-
New issues == only in head
-
Resolved == only in base
-
head issues == codeclimate.json content for MR last commit sha
-
base issues == codeclimate.json content for MR base commit sha
-
if something is present in head but not in base means MR introduce code quality issue that was not present before in master
-
if something is missing in head but was in base means MR fixed code quality issue that was present before in master
Edited by Filipa Lacerda -
184 184 merge_request) 185 185 end 186 186 187 expose :codeclimate do 188 expose :head do |merge_request| 189 if merge_request.head_pipeline 190 build = merge_request.head_pipeline.artifacts.find(&:codeclimate?) 191 192 if build && build.success? 193 raw_namespace_project_build_artifacts_url(merge_request.project.namespace, This URL should be returned only if we do support artifacts browsing, which is not always true. So, currently we don't have such method, but we are looking at
browsable_artifacts?implemented inCi::Build.Also, we should make sure that user can read the build, as not everyone can access artifacts.
-
@ayufan can you help me with this comment? What kind of existing code can I use to make sure we safely return artefact url?
@dzaporozhets Individual files from artifacts can be accessed only when you have metadata:
build.artifacts_metadata?.I wonder if we should not introduce a method:
browsable_artifacts?,extractable_artifacts?or something similar.
added 1 commit
- d0b34e1e - Refactor backend code related to codeclimate widget
-
@ayufan thanks for the review. I am just in progress of cleaning the code since it was made as a concept and not meant for production use.
-
assigned to @iamphill
