diff options
60 files changed, 1121 insertions, 113 deletions
diff --git a/Dangerfile b/Dangerfile index 6a2c5cf2773..715a2bcbbae 100644 --- a/Dangerfile +++ b/Dangerfile @@ -11,3 +11,4 @@ danger.import_dangerfile(path: 'danger/commit_messages') danger.import_dangerfile(path: 'danger/duplicate_yarn_dependencies') danger.import_dangerfile(path: 'danger/prettier') danger.import_dangerfile(path: 'danger/eslint') +danger.import_dangerfile(path: 'danger/roulette') diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 2bf50aaf17a..56b6be4ebb2 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.3.0 +8.3.1 diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 7bbafe66199..5a27388863c 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -144,6 +144,7 @@ export default { if (left || right) { return { + ...line, left: line.left ? mapDiscussions(line.left) : null, right: line.right ? mapDiscussions(line.right, () => !left) : null, }; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index effb6202327..062024b8cdd 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -161,6 +161,7 @@ export function addContextLines(options) { const normalizedParallelLines = contextLines.map(line => ({ left: line, right: line, + line_code: line.line_code, })); if (options.bottom) { diff --git a/app/assets/javascripts/vue_shared/components/file_row.vue b/app/assets/javascripts/vue_shared/components/file_row.vue index f54033efc54..0cbcdbf2eb4 100644 --- a/app/assets/javascripts/vue_shared/components/file_row.vue +++ b/app/assets/javascripts/vue_shared/components/file_row.vue @@ -136,6 +136,7 @@ export default { <div v-else :class="fileClass" + :title="file.name" class="file-row" role="button" @click="clickFile" diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 135730d71e9..c78876d9e2a 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -738,6 +738,8 @@ z-index: 103; background: $gray-light; color: $gl-text-color; + margin-top: -1px; + border-top: 1px solid $border-color; .mr-version-menus-container { display: flex; @@ -789,7 +791,6 @@ position: sticky; top: $header-height + $mr-tabs-height; width: 100%; - border-top: 1px solid $border-color; &.is-fileTreeOpen { margin-left: -16px; @@ -810,10 +811,7 @@ top: $header-height; z-index: 200; background-color: $white-light; - - @include media-breakpoint-down(md) { - border-bottom: 1px solid $border-color; - } + border-bottom: 1px solid $border-color; @include media-breakpoint-up(sm) { position: sticky; diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 9ca54c5519b..28e4cece548 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/app/controllers/concerns/send_file_upload.rb @@ -3,7 +3,7 @@ module SendFileUpload def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, disposition: 'attachment') if attachment - response_disposition = ::Gitlab::ContentDisposition.format(disposition: 'attachment', filename: attachment) + response_disposition = ::Gitlab::ContentDisposition.format(disposition: disposition, filename: attachment) # Response-Content-Type will not override an existing Content-Type in # Google Cloud Storage, so the metadata needs to be cleared on GCS for diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index acef5d2e643..372f6d678f6 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -687,9 +687,18 @@ module Ci end end + # Returns the modified paths. + # + # The returned value is + # * Array: List of modified paths that should be evaluated + # * nil: Modified path can not be evaluated def modified_paths strong_memoize(:modified_paths) do - push_details.modified_paths + if merge_request? + merge_request.modified_paths + elsif branch_updated? + push_details.modified_paths + end end end diff --git a/app/models/releases/link.rb b/app/models/releases/link.rb index 6f639e5a7b2..6c507c47752 100644 --- a/app/models/releases/link.rb +++ b/app/models/releases/link.rb @@ -6,7 +6,7 @@ module Releases belongs_to :release - validates :url, presence: true, url: true, uniqueness: { scope: :release } + validates :url, presence: true, url: { protocols: %w(http https ftp) }, uniqueness: { scope: :release } validates :name, presence: true, uniqueness: { scope: :release } scope :sorted, -> { order(created_at: :desc) } diff --git a/changelogs/unreleased/39676-wiki-api-problems-on-update-parameters-and-500-error.yml b/changelogs/unreleased/39676-wiki-api-problems-on-update-parameters-and-500-error.yml new file mode 100644 index 00000000000..1af49fb6a2c --- /dev/null +++ b/changelogs/unreleased/39676-wiki-api-problems-on-update-parameters-and-500-error.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Require only one parameter when updating a wiki' +merge_request: 25191 +author: Robert Schilling +type: fixed diff --git a/changelogs/unreleased/50006-expose-textcolor-from-public-labels-api.yml b/changelogs/unreleased/50006-expose-textcolor-from-public-labels-api.yml new file mode 100644 index 00000000000..3c8b58f3001 --- /dev/null +++ b/changelogs/unreleased/50006-expose-textcolor-from-public-labels-api.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Expose text_color for project and group labels' +merge_request: 25172 +author: Robert Schilling +type: added diff --git a/changelogs/unreleased/50559-add-milestone-progress-to-api.yml b/changelogs/unreleased/50559-add-milestone-progress-to-api.yml new file mode 100644 index 00000000000..e68e4bd6059 --- /dev/null +++ b/changelogs/unreleased/50559-add-milestone-progress-to-api.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Expose milestone progress' +merge_request: 25173 +author: Robert Schilling +type: added diff --git a/changelogs/unreleased/56237-api-truncated-commit-title.yml b/changelogs/unreleased/56237-api-truncated-commit-title.yml new file mode 100644 index 00000000000..1a48d0fda1b --- /dev/null +++ b/changelogs/unreleased/56237-api-truncated-commit-title.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Expose full commit title' +merge_request: 25189 +author: Robert Schilling +type: fixed diff --git a/changelogs/unreleased/57101-api-docs-for-hangouts-chat-service-incorrect.yml b/changelogs/unreleased/57101-api-docs-for-hangouts-chat-service-incorrect.yml new file mode 100644 index 00000000000..2e0ae9c3732 --- /dev/null +++ b/changelogs/unreleased/57101-api-docs-for-hangouts-chat-service-incorrect.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Fix docs and parameters for hangouts-chat service' +merge_request: 25180 +author: Robert Schilling +type: fixed diff --git a/changelogs/unreleased/57160-merge-request-tabs-header-is-missing-bottom-border.yml b/changelogs/unreleased/57160-merge-request-tabs-header-is-missing-bottom-border.yml new file mode 100644 index 00000000000..3146d07db3d --- /dev/null +++ b/changelogs/unreleased/57160-merge-request-tabs-header-is-missing-bottom-border.yml @@ -0,0 +1,5 @@ +--- +title: Return bottom border on MR Tabs +merge_request: !25198 +author: +type: fixed diff --git a/changelogs/unreleased/57410-api-create-release-link-with-ftp-address-return-400-bad-request.yml b/changelogs/unreleased/57410-api-create-release-link-with-ftp-address-return-400-bad-request.yml new file mode 100644 index 00000000000..6be6a2115b9 --- /dev/null +++ b/changelogs/unreleased/57410-api-create-release-link-with-ftp-address-return-400-bad-request.yml @@ -0,0 +1,5 @@ +--- +title: Add support for FTP assets for releases +merge_request: 25071 +author: Robert Schilling +type: added diff --git a/changelogs/unreleased/57589-update-workhorse.yml b/changelogs/unreleased/57589-update-workhorse.yml new file mode 100644 index 00000000000..525913bba4c --- /dev/null +++ b/changelogs/unreleased/57589-update-workhorse.yml @@ -0,0 +1,5 @@ +--- +title: Update Workhorse to v8.3.1 +merge_request: +author: +type: other diff --git a/changelogs/unreleased/add-title-attribute-to-file-row.yml b/changelogs/unreleased/add-title-attribute-to-file-row.yml new file mode 100644 index 00000000000..c68d3d544e7 --- /dev/null +++ b/changelogs/unreleased/add-title-attribute-to-file-row.yml @@ -0,0 +1,5 @@ +--- +title: add title attribute to display file name +merge_request: 25154 +author: Satoshi Nakamatsu @satoshicano +type: added diff --git a/changelogs/unreleased/support-only-changes-on-mr-pipelines.yml b/changelogs/unreleased/support-only-changes-on-mr-pipelines.yml new file mode 100644 index 00000000000..fbab898b799 --- /dev/null +++ b/changelogs/unreleased/support-only-changes-on-mr-pipelines.yml @@ -0,0 +1,5 @@ +--- +title: 'Support `only: changes:` on MR pipelines' +merge_request: 24490 +author: Hiroyuki Sato +type: added diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile index 530c6638653..63b2f6f5c5c 100644 --- a/danger/changelog/Dangerfile +++ b/danger/changelog/Dangerfile @@ -16,16 +16,12 @@ consider adding any of the %<labels>s labels. #{SEE_DOC} MSG -def ee? - ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md') -end - def ee_changelog?(changelog_path) changelog_path =~ /unreleased-ee/ end def ce_port_changelog?(changelog_path) - ee? && !ee_changelog?(changelog_path) + helper.ee? && !ee_changelog?(changelog_path) end def check_changelog(path) diff --git a/danger/documentation/Dangerfile b/danger/documentation/Dangerfile index 188331cc87c..dd0e3f6deb6 100644 --- a/danger/documentation/Dangerfile +++ b/danger/documentation/Dangerfile @@ -1,17 +1,6 @@ # frozen_string_literal: true -# All the files/directories that should be reviewed by the Docs team. -DOCS_FILES = [ - 'doc/' -].freeze - -def docs_paths_requiring_review(files) - files.select do |file| - DOCS_FILES.any? { |pattern| file.start_with?(pattern) } - end -end - -docs_paths_to_review = docs_paths_requiring_review(helper.all_changed_files) +docs_paths_to_review = helper.changes_by_category[:documentation] unless docs_paths_to_review.empty? message 'This merge request adds or changes files that require a ' \ diff --git a/danger/plugins/helper.rb b/danger/plugins/helper.rb index f4eb9119266..581c0720083 100644 --- a/danger/plugins/helper.rb +++ b/danger/plugins/helper.rb @@ -1,34 +1,15 @@ # frozen_string_literal: true +require 'net/http' +require 'yaml' + +require_relative '../../lib/gitlab/danger/helper' + module Danger - # Common helper functions for our danger scripts - # If we find ourselves repeating code in our danger files, we might as well put them in here. + # Common helper functions for our danger scripts. See Gitlab::Danger::Helper + # for more details class Helper < Plugin - # Returns a list of all files that have been added, modified or renamed. - # `git.modified_files` might contain paths that already have been renamed, - # so we need to remove them from the list. - # - # Considering these changes: - # - # - A new_file.rb - # - D deleted_file.rb - # - M modified_file.rb - # - R renamed_file_before.rb -> renamed_file_after.rb - # - # it will return - # ``` - # [ 'new_file.rb', 'modified_file.rb', 'renamed_file_after.rb' ] - # ``` - # - # @return [Array<String>] - def all_changed_files - Set.new - .merge(git.added_files.to_a) - .merge(git.modified_files.to_a) - .merge(git.renamed_files.map { |x| x[:after] }) - .subtract(git.renamed_files.map { |x| x[:before] }) - .to_a - .sort - end + # Put the helper code somewhere it can be tested + include Gitlab::Danger::Helper end end diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile new file mode 100644 index 00000000000..5c3d7a4ca49 --- /dev/null +++ b/danger/roulette/Dangerfile @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +MESSAGE = <<MARKDOWN +## Reviewer roulette + +Changes that require review have been detected! A merge request is normally +reviewed by both a reviewer and a maintainer in its primary category (e.g. +~frontend or ~backend), and by a maintainer in all other categories. +MARKDOWN + +CATEGORY_TABLE_HEADER = <<MARKDOWN + +To spread load more evenly across eligible reviewers, Danger has randomly picked +a candidate for each review slot. Feel free to override this selection if you +think someone else would be better-suited, or the chosen person is unavailable. + +Once you've decided who will review this merge request, mention them as you +normally would! Danger does not (yet?) automatically notify them for you. + +| Category | Reviewer | Maintainer | +| -------- | -------- | ---------- | +MARKDOWN + +UNKNOWN_FILES_MESSAGE = <<MARKDOWN + +These files couldn't be categorised, so Danger was unable to suggest a reviewer. +Please consider creating a merge request to +[add support](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/danger/helper.rb) +for them. +MARKDOWN + +def spin(team, project, category) + reviewers = team.select { |member| member.reviewer?(project, category) } + maintainers = team.select { |member| member.maintainer?(project, category) } + + # TODO: filter out people who are currently not in the office + # TODO: take CODEOWNERS into account? + + reviewer = reviewers[rand(reviewers.size)] + maintainer = maintainers[rand(maintainers.size)] + + "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name} | #{maintainer&.markdown_name} |" +end + +def build_list(items) + list = items.map { |filename| "* `#{filename}`" }.join("\n") + + if items.size > 10 + "\n<details>\n\n#{list}\n\n</details>" + else + list + end +end + +changes = helper.changes_by_category +categories = changes.keys - [:unknown] + +unless changes.empty? + team = + begin + helper.project_team + rescue => err + warn("Reviewer roulette failed to load team data: #{err.message}") + [] + end + + # Exclude the MR author from the team for selection purposes + team.delete_if { |teammate| teammate.username == gitlab.mr_author } + + project = helper.project_name + unknown = changes.fetch(:unknown, []) + + rows = categories.map { |category| spin(team, project, category) } + + markdown(MESSAGE) + markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty? + markdown(UNKNOWN_FILES_MESSAGE + build_list(unknown)) unless unknown.empty? +end diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 10ae8c7dedf..5c809f25fbd 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -11,7 +11,7 @@ description: 'Learn how to administer GitLab Pages.' > - This guide is for Omnibus GitLab installations. If you have installed > GitLab from source, follow the [Pages source installation document](source.md). > - To learn how to use GitLab Pages, read the [user documentation][pages-userguide]. -> - Does NOT support subgroups. See [this issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/30548) for more information and status. +> - Support for subgroup project's websites was [introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/30548) in GitLab 11.8. This document describes how to set up the _latest_ GitLab Pages feature. Make sure to read the [changelog](#changelog) if you are upgrading to a new GitLab diff --git a/doc/api/group_labels.md b/doc/api/group_labels.md index d4715dec192..3d4b099b49e 100644 --- a/doc/api/group_labels.md +++ b/doc/api/group_labels.md @@ -28,6 +28,7 @@ Example response: "id": 7, "name": "bug", "color": "#FF0000", + "text_color" : "#FFFFFF", "description": null, "open_issues_count": 0, "closed_issues_count": 0, @@ -38,6 +39,7 @@ Example response: "id": 4, "name": "feature", "color": "#228B22", + "text_color" : "#FFFFFF", "description": null, "open_issues_count": 0, "closed_issues_count": 0, @@ -73,6 +75,7 @@ Example response: "id": 9, "name": "Feature Proposal", "color": "#FFA500", + "text_color" : "#FFFFFF", "description": "Describes new ideas", "open_issues_count": 0, "closed_issues_count": 0, @@ -108,6 +111,7 @@ Example response: "id": 9, "name": "Feature Idea", "color": "#FFA500", + "text_color" : "#FFFFFF", "description": "Describes new ideas", "open_issues_count": 0, "closed_issues_count": 0, @@ -158,6 +162,7 @@ Example response: "id": 9, "name": "Feature Idea", "color": "#FFA500", + "text_color" : "#FFFFFF", "description": "Describes new ideas", "open_issues_count": 0, "closed_issues_count": 0, @@ -192,6 +197,7 @@ Example response: "id": 9, "name": "Feature Idea", "color": "#FFA500", + "text_color" : "#FFFFFF", "description": "Describes new ideas", "open_issues_count": 0, "closed_issues_count": 0, diff --git a/doc/api/group_milestones.md b/doc/api/group_milestones.md index 7be01ce9c6d..920acaf3e49 100644 --- a/doc/api/group_milestones.md +++ b/doc/api/group_milestones.md @@ -42,6 +42,7 @@ Example Response: "due_date": "2013-11-29", "start_date": "2013-11-10", "state": "active", + "percentage_complete" : 66, "updated_at": "2013-10-02T09:24:18Z", "created_at": "2013-10-02T09:24:18Z" } diff --git a/doc/api/labels.md b/doc/api/labels.md index aec1a2c7592..5b5b04bc1d7 100644 --- a/doc/api/labels.md +++ b/doc/api/labels.md @@ -24,6 +24,7 @@ Example response: "id" : 1, "name" : "bug", "color" : "#d9534f", + "text_color" : "#FFFFFF", "description": "Bug reported by user", "open_issues_count": 1, "closed_issues_count": 0, @@ -34,6 +35,7 @@ Example response: { "id" : 4, "color" : "#d9534f", + "text_color" : "#FFFFFF", "name" : "confirmed", "description": "Confirmed issue", "open_issues_count": 2, @@ -46,6 +48,7 @@ Example response: "id" : 7, "name" : "critical", "color" : "#d9534f", + "text_color" : "#FFFFFF", "description": "Critical issue. Need fix ASAP", "open_issues_count": 1, "closed_issues_count": 3, @@ -57,6 +60,7 @@ Example response: "id" : 8, "name" : "documentation", "color" : "#f0ad4e", + "text_color" : "#FFFFFF", "description": "Issue about documentation", "open_issues_count": 1, "closed_issues_count": 0, @@ -67,6 +71,7 @@ Example response: { "id" : 9, "color" : "#5cb85c", + "text_color" : "#FFFFFF", "name" : "enhancement", "description": "Enhancement proposal", "open_issues_count": 1, @@ -105,6 +110,7 @@ Example response: "id" : 10, "name" : "feature", "color" : "#5843AD", + "text_color" : "#FFFFFF", "description":null, "open_issues_count": 0, "closed_issues_count": 0, @@ -161,6 +167,7 @@ Example response: "id" : 8, "name" : "docs", "color" : "#8E44AD", + "text_color" : "#FFFFFF", "description": "Documentation", "open_issues_count": 1, "closed_issues_count": 0, @@ -196,6 +203,7 @@ Example response: "id" : 1, "name" : "bug", "color" : "#d9534f", + "text_color" : "#FFFFFF", "description": "Bug reported by user", "open_issues_count": 1, "closed_issues_count": 0, diff --git a/doc/api/milestones.md b/doc/api/milestones.md index fa8f8a0bcf0..21a390442bd 100644 --- a/doc/api/milestones.md +++ b/doc/api/milestones.md @@ -39,6 +39,7 @@ Example Response: "due_date": "2013-11-29", "start_date": "2013-11-10", "state": "active", + "percentage_complete" : 66, "updated_at": "2013-10-02T09:24:18Z", "created_at": "2013-10-02T09:24:18Z" } diff --git a/doc/api/releases/links.md b/doc/api/releases/links.md index ae99f3bd8b6..fd7b9d6e6e2 100644 --- a/doc/api/releases/links.md +++ b/doc/api/releases/links.md @@ -3,6 +3,7 @@ > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/41766) in GitLab 11.7. Using this API you can manipulate GitLab's [Release](../../user/project/releases/index.md) links. For manipulating other Release assets, see [Release API](index.md). +GitLab supports links links to `http`, `https`, and `ftp` assets. ## Get links diff --git a/doc/api/services.md b/doc/api/services.md index 2a8ce39e570..5d5aa3e5b3e 100644 --- a/doc/api/services.md +++ b/doc/api/services.md @@ -412,7 +412,7 @@ Google GSuite team collaboration tool. Set Hangouts Chat service for a project. ``` -PUT /projects/:id/services/hangouts_chat +PUT /projects/:id/services/hangouts-chat ``` >**Note:** Specific event parameters (e.g. `push_events` flag) were [introduced in v10.4][11435] @@ -438,7 +438,7 @@ Parameters: Delete Hangouts Chat service for a project. ``` -DELETE /projects/:id/services/hangouts_chat +DELETE /projects/:id/services/hangouts-chat ``` ### Get Hangouts Chat service settings @@ -446,7 +446,7 @@ DELETE /projects/:id/services/hangouts_chat Get Hangouts Chat service settings for a project. ``` -GET /projects/:id/services/hangouts_chat +GET /projects/:id/services/hangouts-chat ``` ## Irker (IRC gateway) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 984878b6c9b..8c8a31e9323 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -423,10 +423,28 @@ connected with merge requests yet, and because GitLab is creating pipelines before an user can create a merge request we don't know a target branch at this point. -Without a target branch, it is not possible to know what the common ancestor is, -thus we always create a job in that case. This feature works best for stable -branches like `master` because in that case GitLab uses the previous commit -that is present in a branch to compare against the latest SHA that was pushed. +#### Using `changes` with `merge_requests` + +With [pipelines for merge requests](../merge_request_pipelines/index.md), +make it possible to define if a job should be created base on files modified +in a merge request. + +For example: + +``` +docker build service one: + script: docker build -t my-service-one-image:$CI_COMMIT_REF_SLUG . + only: + refs: + - merge_requests + changes: + - Dockerfile + - service-one/**/* +``` + +In the scenario above, if you create or update a merge request that changes +either files in `service-one` folder or `Dockerfile`, GitLab creates and triggers +the `docker build service one` job. ## `tags` diff --git a/doc/development/README.md b/doc/development/README.md index d5829e31343..13646cbfe48 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -49,6 +49,7 @@ description: 'Learn how to contribute to GitLab.' - [Working with the GitHub importer](github_importer.md) - [Import/Export development documentation](import_export.md) - [Working with Merge Request diffs](diffs.md) +- [Kubernetes integration guidelines](kubernetes.md) - [Permissions](permissions.md) - [Prometheus metrics](prometheus_metrics.md) - [Guidelines for reusing abstractions](reusing_abstractions.md) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 25ea2211b64..4e5fb47e331 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -23,6 +23,11 @@ one of the [Merge request coaches][team]. If you need assistance with security scans or comments, feel free to include the Security Team (`@gitlab-com/gl-security`) in the review. +The `danger-review` CI job will randomly pick a reviewer and a maintainer for +each area of the codebase that your merge request seems to touch. It only makes +recommendations - feel free to override it if you think someone else is a better +fit! + Depending on the areas your merge request touches, it must be **approved** by one or more [maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#maintainer): diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md index cda66447c2c..7a3a8f25c2d 100644 --- a/doc/development/documentation/styleguide.md +++ b/doc/development/documentation/styleguide.md @@ -112,7 +112,7 @@ table_display_block: true ## Emphasis - Use double asterisks (`**`) to mark a word or text in bold (`**bold**`). -- Use undescore (`_`) for text in italics (`_italic_`). +- Use underscore (`_`) for text in italics (`_italic_`). - Use greater than (`>`) for blockquotes. ## Punctuation diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 3a3cb77f592..86b8972a69e 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -1,7 +1,7 @@ # Frontend Development Guidelines > **Notice:** -We are currently in the process of re-writing our development guide to make it easier to find information. The new guide is still WIP but viewable in [development/new_fe_guide](../new_fe_guide/index.md) +> We are currently in the process of re-writing our development guide to make it easier to find information. The new guide is still WIP but viewable in [development/new_fe_guide](../new_fe_guide/index.md) This document describes various guidelines to ensure consistency and quality across GitLab's frontend team. @@ -32,32 +32,41 @@ For our currently-supported browsers, see our [requirements][requirements]. --- ## [Development Process](development_process.md) + How we plan and execute the work on the frontend. ## [Architecture](architecture.md) + How we go about making fundamental design decisions in GitLab's frontend team or make changes to our frontend development guidelines. ## [Testing](../testing_guide/frontend_testing.md) + How we write frontend tests, run the GitLab test suite, and debug test related issues. ## [Design Patterns](design_patterns.md) + Common JavaScript design patterns in GitLab's codebase. ## [Vue.js Best Practices](vue.md) + Vue specific design patterns and practices. ## [Vuex](vuex.md) + Vuex specific design patterns and practices. ## [Axios](axios.md) + Axios specific practices and gotchas. ## [GraphQL](graphql.md) + How to use GraphQL ## [Icons and Illustrations](icons.md) + How we use SVG for our Icons and Illustrations. ## [Components](components.md) @@ -70,7 +79,7 @@ How we use UI components. ### [JavaScript Style Guide](style_guide_js.md) -We use eslint to enforce our JavaScript style guides. Our guide is based on +We use eslint to enforce our JavaScript style guides. Our guide is based on the excellent [Airbnb][airbnb-js-style-guide] style guide with a few small changes. @@ -81,23 +90,26 @@ Our SCSS conventions which are enforced through [scss-lint][scss-lint]. --- ## [Performance](performance.md) + Best practices for monitoring and maximizing frontend performance. --- ## [Security](security.md) + Frontend security practices. --- ## [Accessibility](accessibility.md) + Our accessibility standards and resources. ## [Internationalization (i18n) and Translations](../i18n/externalization.md) + Frontend internationalization support is described in [this document](../i18n/). The [externalization part of the guide](../i18n/externalization.md) explains the helpers/methods available. - [rails]: http://rubyonrails.org/ [haml]: http://haml.info/ [hamlit]: https://github.com/k0kubun/hamlit @@ -116,6 +128,7 @@ The [externalization part of the guide](../i18n/externalization.md) explains the --- ## [DropLab](droplab/droplab.md) + Our internal `DropLab` dropdown library. - [DropLab](droplab/droplab.md) diff --git a/doc/development/kubernetes.md b/doc/development/kubernetes.md new file mode 100644 index 00000000000..4b2d48903ac --- /dev/null +++ b/doc/development/kubernetes.md @@ -0,0 +1,126 @@ +# Kubernetes integration - development guidelines + +This document provides various guidelines when developing for GitLab's +[Kubernetes integration](../user/project/clusters/index.md). + +## Development + +### Architecture + +Some Kubernetes operations, such as creating restricted project +namespaces are performed on the GitLab Rails application. These +operations are performed using a [client library](#client-library). +These operations will carry an element of risk as the operations will be +run as the same user running the GitLab Rails application, see the +[security](#security) section below. + +Some Kubernetes operations, such as installing cluster applications are +performed on one-off pods on the Kubernetes cluster itself. These +installation pods are currently named `install-<application_name>` and +are created within the `gitlab-managed-apps` namespace. + +In terms of code organization, we generally add objects that represent +Kubernetes resources in +[`lib/gitlab/kubernetes`](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/lib/gitlab/kubernetes). + +### Client library + +We use the [`kubeclient`](https://rubygems.org/gems/kubeclient) gem to +perform Kubernetes API calls. As the `kubeclient` gem does not support +different API Groups (e.g. `apis/rbac.authorization.k8s.io`) from a +single client, we have created a wrapper class, +[`Gitlab::Kubernetes::KubeClient`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/kubernetes/kube_client.rb) +that will enable you to achieve this. + +Selected Kubernetes API groups are currently supported. Do add support +for new API groups or methods to +[`Gitlab::Kubernetes::KubeClient`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/kubernetes/kube_client.rb) +if you need to use them. New API groups or API group versions can be +added to `SUPPORTED_API_GROUPS` - internally, this will create an +internal client for that group. New methods can be added as a delegation +to the relevant internal client. + +### Performance considerations + +All calls to the Kubernetes API must be in a background process. Do not +perform Kubernetes API calls within a web request as this will block +unicorn and can easily lead to a Denial Of Service (DoS) attack in GitLab as +the Kubernetes cluster response times are outside of our control. + +The easiest way to ensure your calls happen a background process is to +delegate any such work to happen in a [sidekiq +worker](sidekiq_style_guide.md). + +There are instances where you would like to make calls to Kubernetes and +return the response and as such a background worker does not seem to be +a good fit. For such cases you should make use of [reactive +caching](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/models/concerns/reactive_caching.rb). +For example: + +```ruby + def calculate_reactive_cache! + { pods: cluster.platform_kubernetes.kubeclient.get_pods } + end + + def pods + with_reactive_cache do |data| + data[:pods] + end + end +``` + +### Testing + +We have some Webmock stubs in +[`KubernetesHelpers`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/helpers/kubernetes_helpers.rb) +which can help with mocking out calls to Kubernetes API in your tests. + +## Security + +### SSRF + +As URLs for Kubernetes clusters are user controlled it is easily +susceptible to Server Side Request Forgery (SSRF) attacks. You should +understand the mitigation strategies if you are adding more API calls to +a cluster. + +Mitigation strategies include: + +1. Not allowing redirects to attacker controller resources: + [`Kubeclient::KubeClient`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/kubernetes/kube_client.rb#) + can be configured to disallow any redirects by passing in + `http_max_redirects: 0` as an option. +1. Not exposing error messages: by doing so, we + prevent attackers from triggering errors to expose results from + attacker controlled requests. For example, we do not expose (or store) + raw error messages: + + ```ruby + rescue Kubernetes::HttpError => e + # bad + # app.make_errored!("Kubernetes error: #{e.message}") + + # good + app.make_errored!("Kubernetes error: #{e.error_code}") + ``` + +## Debugging + +Logs related to the Kubernetes integration can be found in +[kubernetes.log](../administration/logs.md#kuberneteslog). On a local +GDK install, this will be present in `log/kubernetes.log`. + +Some services such as +[`Clusters::Applications::InstallService`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/services/clusters/applications/install_service.rb#L18) +rescues `StandardError` which can make it harder to debug issues in an +development environment. The current workaround is to temporarily +comment out the `rescue` in your local development source. + +You can also follow the installation pod logs to debug issues related to +installation. Once the installation/upgrade is underway, wait for the +pod to be created. Then run the following to obtain the pods logs as +they are written: + +```bash +kubectl logs <pod_name> --follow -n gitlab-managed-apps +``` diff --git a/doc/development/new_fe_guide/event_tracking.md b/doc/development/new_fe_guide/event_tracking.md new file mode 100644 index 00000000000..1958f1ce528 --- /dev/null +++ b/doc/development/new_fe_guide/event_tracking.md @@ -0,0 +1,74 @@ +# Event Tracking + +We use [Snowplow](https://github.com/snowplow/snowplow) for tracking custom events. + +## Generic tracking function + +In addition to Snowplow's built-in method for tracking page views, we use a generic tracking function which enables us to selectively apply listeners to events. + +The generic tracking function can be imported in EE-specific JS files as follows: + +```javascript +import { trackEvent } from `ee/stats`; +``` + +This gives the user access to the `trackEvent` method, which takes the following parameters: + +| parameter | type | description | required | +| ---------------- | ------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | +| `category` | string | Describes the page that you're capturing click events on. Unless infeasible, please use the Rails page attribute `document.body.dataset.page` by default. | true | +| `eventName` | string | Describes the action the user is taking. The first word should always describe the action. For example, clicks should be `click` and activations should be `activate`. Use underscores to describe what was acted on. For example, activating a form field would be `activate_form_input`. Clicking on a dropdown is `click_dropdown`. | true | +| `additionalData` | object | Additional data such as `label`, `property`, and `value` as described [in our Feature Instrumentation taxonomy](https://about.gitlab.com/handbook/product/feature-instrumentation/#taxonomy). | false | + +Read more about instrumentation and the taxonomy in the [Product Handbook](https://about.gitlab.com/handbook/product/feature-instrumentation). + +### Tracking in `.js` and `.vue` files + +The most simple use case is to add tracking programmatically to an event of interest in Javascript. + +The following example demonstrates how to track a click on a button in Javascript by calling the `trackEvent` method explicitly: + +```javascript +import { trackEvent } from `ee/stats`; + +trackEvent('dashboard:projects:index', 'click_button', { + label: 'create_from_template', + property: 'template_preview', + value: 'rails', +}); +``` + +### Tracking in HAML templates + +Sometimes we want to track clicks for multiple elements on a page. Creating event handlers for all elements could soon turn into a tedious task. + +There's a more convenient solution to this problem. When working with HAML templates, we can add `data-track-*` attributes to elements of interest. This way, all elements that have both `data-track-label` and `data-track-event` attributes assigned get marked for event tracking. All we have to do is call the `bindTrackableContainer` method on a container which allows for better scoping. + +Below is an example of `data-track-*` attributes assigned to a button in HAML: + +```ruby +%button.btn{ data: { track_label: "create_from_template", track_property: "template_preview", track_event: "click_button", track_value: "my-template" } } +``` + +By calling `bindTrackableContainer('.my-container')`, click handlers get bound to all elements located in `.my-container` provided that they have the necessary `data-track-*` attributes assigned to them. + +```javascript +import Stats from 'ee/stats'; + +document.addEventListener('DOMContentLoaded', () => { + Stats.bindTrackableContainer('.my-container', 'category'); +}); +``` + +The second parameter in `bindTrackableContainer` is optional. If omitted, the value of `document.body.dataset.page` will be used as category instead. + +Below is a list of supported `data-track-*` attributes: + +| attribute | description | required | +| --------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | +| `data-track-label` | The `label` in `trackEvent` | true | +| `data-track-event` | The `eventName` in `trackEvent` | true | +| `data-track-property` | The `property` in `trackEvent`. If omitted, an empty string will be used as a default value. | false | +| `data-track-value` | The `value` in `trackEvent`. If omitted, this will be `target.value` or empty string. For checkboxes, the default value being tracked will be the element's checked attribute if `data-track-value` is omitted. | false | + +Since Snowplow is an Enterprise Edition feature, it's necessary to create a CE backport when adding `data-track-*` attributes to HAML templates in most cases. diff --git a/doc/development/new_fe_guide/index.md b/doc/development/new_fe_guide/index.md index bfcca9cec7b..5fd5af252ef 100644 --- a/doc/development/new_fe_guide/index.md +++ b/doc/development/new_fe_guide/index.md @@ -27,6 +27,10 @@ Learn about all the internal JavaScript modules that make up our frontend. Style guides to keep our code consistent. +## [Event Tracking with Snowplow](event_tracking.md) + +How we use Snowplow to track custom events. + ## [Tips](tips.md) Tips from our frontend team to develop more efficiently and effectively. diff --git a/doc/user/project/pages/index.md b/doc/user/project/pages/index.md index 11f6165fcb4..2de3fb7e080 100644 --- a/doc/user/project/pages/index.md +++ b/doc/user/project/pages/index.md @@ -66,7 +66,7 @@ publish any website written directly in plain HTML, CSS, and JavaScript.</p> If you're using GitLab.com, your website will be publicly available to the internet. If you're using self-managed instances (Core, Starter, Premium, or Ultimate), your websites will be published on your own server, according to the -[Pages admin settings](../../../administration/pages/index.md) chosen by your sysdamin, +[Pages admin settings](../../../administration/pages/index.md) chosen by your sysadmin, who can opt for making them public or internal to your server. ### How it works diff --git a/doc/user/project/web_ide/index.md b/doc/user/project/web_ide/index.md index 9c095da5a4b..3e85e97d7a5 100644 --- a/doc/user/project/web_ide/index.md +++ b/doc/user/project/web_ide/index.md @@ -50,7 +50,7 @@ review the list of changed files. Click on each file to review the changes and click the tick icon to stage the file. Once you have staged some changes, you can add a commit message and commit the -staged changes. Unstaged changes will not be commited. +staged changes. Unstaged changes will not be committed.  diff --git a/lib/api/entities.rb b/lib/api/entities.rb index beb8ce349b4..0ef56067b95 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -369,8 +369,9 @@ module API end class Commit < Grape::Entity - expose :id, :short_id, :title, :created_at + expose :id, :short_id, :created_at expose :parent_ids + expose :full_title, as: :title expose :safe_message, as: :message expose :author_name, :author_email, :authored_date expose :committer_name, :committer_email, :committed_date @@ -501,6 +502,9 @@ module API expose :state, :created_at, :updated_at expose :due_date expose :start_date + expose :percentage_complete do |milestone, options| + milestone.percent_complete(options[:current_user]) + end expose :web_url do |milestone, _options| Gitlab::UrlBuilder.build(milestone) @@ -1003,7 +1007,7 @@ module API end class LabelBasic < Grape::Entity - expose :id, :name, :color, :description + expose :id, :name, :color, :description, :text_color end class Label < LabelBasic diff --git a/lib/api/milestone_responses.rb b/lib/api/milestone_responses.rb index a0ca39b69d4..4c68c568aaa 100644 --- a/lib/api/milestone_responses.rb +++ b/lib/api/milestone_responses.rb @@ -35,19 +35,19 @@ module API milestones = filter_by_iid(milestones, params[:iids]) if params[:iids].present? milestones = filter_by_search(milestones, params[:search]) if params[:search] - present paginate(milestones), with: Entities::Milestone + present paginate(milestones), with: Entities::Milestone, current_user: current_user end def get_milestone_for(parent) milestone = parent.milestones.find(params[:milestone_id]) - present milestone, with: Entities::Milestone + present milestone, with: Entities::Milestone, current_user: current_user end def create_milestone_for(parent) milestone = ::Milestones::CreateService.new(parent, current_user, declared_params).execute if milestone.valid? - present milestone, with: Entities::Milestone + present milestone, with: Entities::Milestone, current_user: current_user else render_api_error!("Failed to create milestone #{milestone.errors.messages}", 400) end @@ -60,7 +60,7 @@ module API milestone = ::Milestones::UpdateService.new(parent, current_user, milestone_params).execute(milestone) if milestone.valid? - present milestone, with: Entities::Milestone + present milestone, with: Entities::Milestone, current_user: current_user else render_api_error!("Failed to update milestone #{milestone.errors.messages}", 400) end diff --git a/lib/api/services.rb b/lib/api/services.rb index 36bdba2d765..163c7505a65 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -368,8 +368,9 @@ module API name: :webhook, type: String, desc: 'The Hangouts Chat webhook. e.g. https://chat.googleapis.com/v1/spaces…' - } - ], + }, + CHAT_NOTIFICATION_EVENTS + ].flatten, 'irker' => [ { required: true, diff --git a/lib/api/wikis.rb b/lib/api/wikis.rb index ef0e3decc2c..994074ddc67 100644 --- a/lib/api/wikis.rb +++ b/lib/api/wikis.rb @@ -11,9 +11,7 @@ module API } end - params :wiki_page_params do - requires :content, type: String, desc: 'Content of a wiki page' - requires :title, type: String, desc: 'Title of a wiki page' + params :common_wiki_page_params do optional :format, type: String, values: ProjectWiki::MARKUPS.values.map(&:to_s), @@ -54,7 +52,9 @@ module API success Entities::WikiPage end params do - use :wiki_page_params + requires :title, type: String, desc: 'Title of a wiki page' + requires :content, type: String, desc: 'Content of a wiki page' + use :common_wiki_page_params end post ':id/wikis' do authorize! :create_wiki, user_project @@ -72,7 +72,10 @@ module API success Entities::WikiPage end params do - use :wiki_page_params + optional :title, type: String, desc: 'Title of a wiki page' + optional :content, type: String, desc: 'Content of a wiki page' + use :common_wiki_page_params + at_least_one_of :content, :title, :format end put ':id/wikis/:slug' do authorize! :create_wiki, user_project diff --git a/lib/gitlab/ci/build/policy/changes.rb b/lib/gitlab/ci/build/policy/changes.rb index 1663c875426..9c705a1cd3e 100644 --- a/lib/gitlab/ci/build/policy/changes.rb +++ b/lib/gitlab/ci/build/policy/changes.rb @@ -10,7 +10,7 @@ module Gitlab end def satisfied_by?(pipeline, seed) - return true unless pipeline.branch_updated? + return true if pipeline.modified_paths.nil? pipeline.modified_paths.any? do |path| @globs.any? do |glob| diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb new file mode 100644 index 00000000000..80760e41c7d --- /dev/null +++ b/lib/gitlab/danger/helper.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true +require 'net/http' +require 'json' + +require_relative 'teammate' + +module Gitlab + module Danger + module Helper + ROULETTE_DATA_URL = URI.parse('https://about.gitlab.com/roulette.json').freeze + + # Returns a list of all files that have been added, modified or renamed. + # `git.modified_files` might contain paths that already have been renamed, + # so we need to remove them from the list. + # + # Considering these changes: + # + # - A new_file.rb + # - D deleted_file.rb + # - M modified_file.rb + # - R renamed_file_before.rb -> renamed_file_after.rb + # + # it will return + # ``` + # [ 'new_file.rb', 'modified_file.rb', 'renamed_file_after.rb' ] + # ``` + # + # @return [Array<String>] + def all_changed_files + Set.new + .merge(git.added_files.to_a) + .merge(git.modified_files.to_a) + .merge(git.renamed_files.map { |x| x[:after] }) + .subtract(git.renamed_files.map { |x| x[:before] }) + .to_a + .sort + end + + def ee? + ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md') + end + + def project_name + ee? ? 'gitlab-ee' : 'gitlab-ce' + end + + # Looks up the current list of GitLab team members and parses it into a + # useful form + # + # @return [Array<Teammate>] + def team + @team ||= + begin + rsp = Net::HTTP.get_response(ROULETTE_DATA_URL) + raise "Failed to read #{ROULETTE_DATA_URL}: #{rsp.code} #{rsp.message}" unless + rsp.is_a?(Net::HTTPSuccess) + + data = JSON.parse(rsp.body) + data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) } + rescue JSON::ParserError + raise "Failed to parse JSON response from #{ROULETTE_DATA_URL}" + end + end + + # Like +team+, but only returns teammates in the current project, based on + # project_name. + # + # @return [Array<Teammate>] + def project_team + team.select { |member| member.in_project?(project_name) } + end + + # @return [Hash<String,Array<String>>] + def changes_by_category + all_changed_files.each_with_object(Hash.new { |h, k| h[k] = [] }) do |file, hash| + hash[category_for_file(file)] << file + end + end + + # Determines the category a file is in, e.g., `:frontend` or `:backend` + # @return[Symbol] + def category_for_file(file) + _, category = CATEGORIES.find { |regexp, _| regexp.match?(file) } + + category || :unknown + end + + # Returns the GFM for a category label, making its best guess if it's not + # a category we know about. + # + # @return[String] + def label_for_category(category) + CATEGORY_LABELS.fetch(category, "~#{category}") + end + + CATEGORY_LABELS = { + docs: "~Documentation", + qa: "~QA" + }.freeze + + # rubocop:disable Style/RegexpLiteral + CATEGORIES = { + %r{\Adoc/} => :docs, + %r{\A(CONTRIBUTING|LICENSE|MAINTENANCE|PHILOSOPHY|PROCESS|README)(\.md)?\z} => :docs, + + %r{\A(ee/)?app/(assets|views)/} => :frontend, + %r{\A(ee/)?public/} => :frontend, + %r{\A(ee/)?spec/javascripts/} => :frontend, + %r{\A(ee/)?vendor/assets/} => :frontend, + %r{\A(jest\.config\.js|package\.json|yarn\.lock)\z} => :frontend, + + %r{\A(ee/)?app/(?!assets|views)[^/]+} => :backend, + %r{\A(ee/)?(bin|config|danger|generator_templates|lib|rubocop|scripts)/} => :backend, + %r{\A(ee/)?spec/(?!javascripts)[^/]+} => :backend, + %r{\A(ee/)?vendor/(?!assets)[^/]+} => :backend, + %r{\A(ee/)?vendor/(languages\.yml|licenses\.csv)\z} => :backend, + %r{\A(Dangerfile|Gemfile|Gemfile.lock|Procfile|Rakefile)\z} => :backend, + %r{\A[A-Z_]+_VERSION\z} => :backend, + + %r{\A(ee/)?db/} => :database, + %r{\A(ee/)?qa/} => :qa, + + # Fallbacks in case the above patterns miss anything + %r{\.rb\z} => :backend, + %r{\.(md|txt)\z} => :docs, + %r{\.js\z} => :frontend + }.freeze + # rubocop:enable Style/RegexpLiteral + end + end +end diff --git a/lib/gitlab/danger/teammate.rb b/lib/gitlab/danger/teammate.rb new file mode 100644 index 00000000000..4b822aa86c5 --- /dev/null +++ b/lib/gitlab/danger/teammate.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module Gitlab + module Danger + class Teammate + attr_reader :name, :username, :projects + + def initialize(options = {}) + @name = options['name'] + @username = options['username'] + @projects = options['projects'] + end + + def markdown_name + "[#{name}](https://gitlab.com/#{username}) (`@#{username}`)" + end + + def in_project?(name) + projects&.has_key?(name) + end + + # Traintainers also count as reviewers + def reviewer?(project, category) + capabilities(project) == "reviewer #{category}" || traintainer?(project, category) + end + + def traintainer?(project, category) + capabilities(project) == "trainee_maintainer #{category}" + end + + def maintainer?(project, category) + capabilities(project) == "maintainer #{category}" + end + + private + + def capabilities(project) + projects.fetch(project, '') + end + end + end +end diff --git a/spec/controllers/concerns/send_file_upload_spec.rb b/spec/controllers/concerns/send_file_upload_spec.rb index a07113a6156..cf3b24f50a3 100644 --- a/spec/controllers/concerns/send_file_upload_spec.rb +++ b/spec/controllers/concerns/send_file_upload_spec.rb @@ -52,6 +52,23 @@ describe SendFileUpload do end end + context 'with inline image' do + let(:filename) { 'test.png' } + let(:params) { { disposition: 'inline', attachment: filename } } + + it 'sends a file with inline disposition' do + # Notice the filename= is omitted from the disposition; this is because + # Rails 5 will append this header in send_file + expected_params = { + filename: 'test.png', + disposition: "inline; filename*=UTF-8''test.png" + } + expect(controller).to receive(:send_file).with(uploader.path, expected_params) + + subject + end + end + context 'with attachment' do let(:filename) { 'test.js' } let(:params) { { attachment: filename } } diff --git a/spec/fixtures/api/schemas/public_api/v4/group_labels.json b/spec/fixtures/api/schemas/public_api/v4/group_labels.json index f6c327abfdd..fbde45f2904 100644 --- a/spec/fixtures/api/schemas/public_api/v4/group_labels.json +++ b/spec/fixtures/api/schemas/public_api/v4/group_labels.json @@ -6,6 +6,7 @@ "id" : { "type": "integer" }, "name" : { "type": "string "}, "color" : { "type": "string "}, + "text_color" : { "type": "string "}, "description" : { "type": "string "}, "open_issues_count" : { "type": "integer "}, "closed_issues_count" : { "type": "integer "}, diff --git a/spec/fixtures/api/schemas/public_api/v4/milestone.json b/spec/fixtures/api/schemas/public_api/v4/milestone.json index 6ca2e88ae91..971f7980f46 100644 --- a/spec/fixtures/api/schemas/public_api/v4/milestone.json +++ b/spec/fixtures/api/schemas/public_api/v4/milestone.json @@ -8,6 +8,7 @@ "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, + "percentage_complete": { "type": "integer" }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, "start_date": { "type": "date" }, diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index c5e413a29d8..baf6e111f9f 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -14,7 +14,7 @@ import { MERGE_REQUEST_NOTEABLE_TYPE } from '~/notes/constants'; import diffFileMockData from '../mock_data/diff_file'; import { noteableDataMock } from '../../notes/mock_data'; -const getDiffFileMock = () => Object.assign({}, diffFileMockData); +const getDiffFileMock = () => JSON.parse(JSON.stringify(diffFileMockData)); describe('DiffsStoreUtils', () => { describe('findDiffFile', () => { @@ -80,30 +80,44 @@ describe('DiffsStoreUtils', () => { }); describe('addContextLines', () => { - it('should add context lines properly with bottom parameter', () => { + it('should add context lines', () => { const diffFile = getDiffFileMock(); const inlineLines = diffFile.highlighted_diff_lines; const parallelLines = diffFile.parallel_diff_lines; const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 }; - const contextLines = [{ lineNumber: 42 }]; - const options = { inlineLines, parallelLines, contextLines, lineNumbers, bottom: true }; + const contextLines = [{ lineNumber: 42, line_code: '123' }]; + const options = { inlineLines, parallelLines, contextLines, lineNumbers }; const inlineIndex = utils.findIndexInInlineLines(inlineLines, lineNumbers); const parallelIndex = utils.findIndexInParallelLines(parallelLines, lineNumbers); const normalizedParallelLine = { left: options.contextLines[0], right: options.contextLines[0], + line_code: '123', }; utils.addContextLines(options); - expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]); - expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine); + expect(inlineLines[inlineIndex]).toEqual(contextLines[0]); + expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine); + }); + + it('should add context lines properly with bottom parameter', () => { + const diffFile = getDiffFileMock(); + const inlineLines = diffFile.highlighted_diff_lines; + const parallelLines = diffFile.parallel_diff_lines; + const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 }; + const contextLines = [{ lineNumber: 42, line_code: '123' }]; + const options = { inlineLines, parallelLines, contextLines, lineNumbers, bottom: true }; + const normalizedParallelLine = { + left: options.contextLines[0], + right: options.contextLines[0], + line_code: '123', + }; - delete options.bottom; utils.addContextLines(options); - expect(inlineLines[inlineIndex]).toEqual(contextLines[0]); - expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine); + expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]); + expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine); }); }); diff --git a/spec/lib/gitlab/ci/build/policy/changes_spec.rb b/spec/lib/gitlab/ci/build/policy/changes_spec.rb index 5fee37bb43e..dc3329061d1 100644 --- a/spec/lib/gitlab/ci/build/policy/changes_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/changes_spec.rb @@ -73,9 +73,9 @@ describe Gitlab::Ci::Build::Policy::Changes do expect(policy).not_to be_satisfied_by(pipeline, seed) end - context 'when pipelines does not run for a branch update' do + context 'when modified paths can not be evaluated' do before do - pipeline.before_sha = Gitlab::Git::BLANK_SHA + allow(pipeline).to receive(:modified_paths) { nil } end it 'is always satisfied' do @@ -115,5 +115,57 @@ describe Gitlab::Ci::Build::Policy::Changes do expect(policy).not_to be_satisfied_by(pipeline, seed) end end + + context 'when branch is created' do + let(:pipeline) do + create(:ci_empty_pipeline, project: project, + ref: 'feature', + source: source, + sha: '0b4bc9a4', + before_sha: Gitlab::Git::BLANK_SHA, + merge_request: merge_request) + end + + let(:ci_build) do + build(:ci_build, pipeline: pipeline, project: project, ref: 'feature') + end + + let(:seed) { double('build seed', to_resource: ci_build) } + + context 'when source is merge request' do + let(:source) { :merge_request } + + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'feature', + target_project: project, + target_branch: 'master') + end + + it 'is satified by changes in the merge request' do + policy = described_class.new(%w[files/ruby/feature.rb]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is not satified by changes not in the merge request' do + policy = described_class.new(%w[foo.rb]) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + end + + context 'when source is push' do + let(:source) { :push } + let(:merge_request) { nil } + + it 'is always satified' do + policy = described_class.new(%w[foo.rb]) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + end + end end end diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb new file mode 100644 index 00000000000..75080aacd96 --- /dev/null +++ b/spec/lib/gitlab/danger/helper_spec.rb @@ -0,0 +1,294 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' +require 'webmock/rspec' + +require 'gitlab/danger/helper' + +describe Gitlab::Danger::Helper do + using RSpec::Parameterized::TableSyntax + + class FakeDanger + include Gitlab::Danger::Helper + + attr_reader :git + + def initialize(git:) + @git = git + end + end + + let(:teammate_json) do + <<~JSON + [ + { + "username": "in-gitlab-ce", + "name": "CE maintainer", + "projects":{ "gitlab-ce": "maintainer backend" } + }, + { + "username": "in-gitlab-ee", + "name": "EE reviewer", + "projects":{ "gitlab-ee": "reviewer frontend" } + } + ] + JSON + end + + let(:ce_teammate_matcher) do + satisfy do |teammate| + teammate.username == 'in-gitlab-ce' && + teammate.name == 'CE maintainer' && + teammate.projects == { 'gitlab-ce' => 'maintainer backend' } + end + end + + let(:ee_teammate_matcher) do + satisfy do |teammate| + teammate.username == 'in-gitlab-ee' && + teammate.name == 'EE reviewer' && + teammate.projects == { 'gitlab-ee' => 'reviewer frontend' } + end + end + + let(:fake_git) { double('fake-git') } + + subject(:helper) { FakeDanger.new(git: fake_git) } + + describe '#all_changed_files' do + subject { helper.all_changed_files } + + it 'interprets a list of changes from the danger git plugin' do + expect(fake_git).to receive(:added_files) { %w[a b c.old] } + expect(fake_git).to receive(:modified_files) { %w[d e] } + expect(fake_git) + .to receive(:renamed_files) + .at_least(:once) + .and_return([{ before: 'c.old', after: 'c.new' }]) + + is_expected.to contain_exactly('a', 'b', 'c.new', 'd', 'e') + end + end + + describe '#ee?' do + subject { helper.ee? } + + it 'returns true if CI_PROJECT_NAME if set to gitlab-ee' do + stub_env('CI_PROJECT_NAME', 'gitlab-ee') + expect(File).not_to receive(:exist?) + + is_expected.to be_truthy + end + + it 'delegates to CHANGELOG-EE.md existence if CI_PROJECT_NAME is set to something else' do + stub_env('CI_PROJECT_NAME', 'something else') + expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { true } + + is_expected.to be_truthy + end + + it 'returns true if CHANGELOG-EE.md exists' do + stub_env('CI_PROJECT_NAME', nil) + expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { true } + + is_expected.to be_truthy + end + + it "returns false if CHANGELOG-EE.md doesn't exist" do + stub_env('CI_PROJECT_NAME', nil) + expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { false } + + is_expected.to be_falsy + end + end + + describe '#project_name' do + subject { helper.project_name } + + it 'returns gitlab-ee if ee? returns true' do + expect(helper).to receive(:ee?) { true } + + is_expected.to eq('gitlab-ee') + end + + it 'returns gitlab-ce if ee? returns false' do + expect(helper).to receive(:ee?) { false } + + is_expected.to eq('gitlab-ce') + end + end + + describe '#team' do + subject(:team) { helper.team } + + context 'HTTP failure' do + before do + WebMock + .stub_request(:get, 'https://about.gitlab.com/roulette.json') + .to_return(status: 404) + end + + it 'raises a pretty error' do + expect { team }.to raise_error(/Failed to read/) + end + end + + context 'JSON failure' do + before do + WebMock + .stub_request(:get, 'https://about.gitlab.com/roulette.json') + .to_return(body: 'INVALID JSON') + end + + it 'raises a pretty error' do + expect { team }.to raise_error(/Failed to parse/) + end + end + + context 'success' do + before do + WebMock + .stub_request(:get, 'https://about.gitlab.com/roulette.json') + .to_return(body: teammate_json) + end + + it 'returns an array of teammates' do + is_expected.to contain_exactly(ce_teammate_matcher, ee_teammate_matcher) + end + + it 'memoizes the result' do + expect(team.object_id).to eq(helper.team.object_id) + end + end + end + + describe '#project_team' do + subject { helper.project_team } + + before do + WebMock + .stub_request(:get, 'https://about.gitlab.com/roulette.json') + .to_return(body: teammate_json) + end + + it 'filters team by project_name' do + expect(helper) + .to receive(:project_name) + .at_least(:once) + .and_return('gitlab-ce') + + is_expected.to contain_exactly(ce_teammate_matcher) + end + end + + describe '#changes_by_category' do + it 'categorizes changed files' do + expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/foo qa/foo] } + allow(fake_git).to receive(:modified_files) { [] } + allow(fake_git).to receive(:renamed_files) { [] } + + expect(helper.changes_by_category).to eq( + backend: %w[foo.rb], + database: %w[db/foo], + docs: %w[foo.md], + frontend: %w[foo.js], + qa: %w[qa/foo], + unknown: %w[foo] + ) + end + end + + describe '#category_for_file' do + where(:path, :expected_category) do + 'doc/foo' | :docs + 'CONTRIBUTING.md' | :docs + 'LICENSE' | :docs + 'MAINTENANCE.md' | :docs + 'PHILOSOPHY.md' | :docs + 'PROCESS.md' | :docs + 'README.md' | :docs + + 'ee/doc/foo' | :unknown + 'ee/README' | :unknown + + 'app/assets/foo' | :frontend + 'app/views/foo' | :frontend + 'public/foo' | :frontend + 'spec/javascripts/foo' | :frontend + 'vendor/assets/foo' | :frontend + 'jest.config.js' | :frontend + 'package.json' | :frontend + 'yarn.lock' | :frontend + + 'ee/app/assets/foo' | :frontend + 'ee/app/views/foo' | :frontend + 'ee/spec/javascripts/foo' | :frontend + + 'app/models/foo' | :backend + 'bin/foo' | :backend + 'config/foo' | :backend + 'danger/foo' | :backend + 'lib/foo' | :backend + 'rubocop/foo' | :backend + 'scripts/foo' | :backend + 'spec/foo' | :backend + 'spec/foo/bar' | :backend + + 'ee/app/foo' | :backend + 'ee/bin/foo' | :backend + 'ee/spec/foo' | :backend + 'ee/spec/foo/bar' | :backend + + 'generator_templates/foo' | :backend + 'vendor/languages.yml' | :backend + 'vendor/licenses.csv' | :backend + + 'Dangerfile' | :backend + 'Gemfile' | :backend + 'Gemfile.lock' | :backend + 'Procfile' | :backend + 'Rakefile' | :backend + 'FOO_VERSION' | :backend + + 'ee/FOO_VERSION' | :unknown + + 'db/foo' | :database + 'qa/foo' | :qa + + 'ee/db/foo' | :database + 'ee/qa/foo' | :qa + + 'FOO' | :unknown + 'foo' | :unknown + + 'foo/bar.rb' | :backend + 'foo/bar.js' | :frontend + 'foo/bar.txt' | :docs + 'foo/bar.md' | :docs + end + + with_them do + subject { helper.category_for_file(path) } + + it { is_expected.to eq(expected_category) } + end + end + + describe '#label_for_category' do + where(:category, :expected_label) do + :backend | '~backend' + :database | '~database' + :docs | '~Documentation' + :foo | '~foo' + :frontend | '~frontend' + :qa | '~QA' + end + + with_them do + subject { helper.label_for_category(category) } + + it { is_expected.to eq(expected_label) } + end + end +end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 72a0df96a80..460b5c8cd31 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1172,8 +1172,26 @@ describe Ci::Pipeline, :mailer do pipeline.update_column(:before_sha, Gitlab::Git::BLANK_SHA) end - it 'raises an error' do - expect { pipeline.modified_paths }.to raise_error(ArgumentError) + it 'returns nil' do + expect(pipeline.modified_paths).to be_nil + end + end + + context 'when source is merge request' do + let(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request) + end + + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'feature', + target_project: project, + target_branch: 'master') + end + + it 'returns merge request modified paths' do + expect(pipeline.modified_paths).to match(merge_request.modified_paths) end end end diff --git a/spec/models/releases/link_spec.rb b/spec/models/releases/link_spec.rb index 06ed1438688..4dd26c976cc 100644 --- a/spec/models/releases/link_spec.rb +++ b/spec/models/releases/link_spec.rb @@ -77,4 +77,28 @@ describe Releases::Link do it { is_expected.to be_truthy } end + + describe 'supported protocols' do + where(:protocol) do + %w(http https ftp) + end + + with_them do + let(:link) { build(:release_link, url: protocol + '://assets.com/download') } + + it 'will be valid' do + expect(link).to be_valid + end + end + end + + describe 'unsupported protocol' do + context 'for torrent' do + let(:link) { build(:release_link, url: 'torrent://assets.com/download') } + + it 'will be invalid' do + expect(link).to be_invalid + end + end + end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 6b9bc6eda6a..bc892523cf8 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -237,7 +237,7 @@ describe API::Commits do end describe 'create' do - let(:message) { 'Created file' } + let(:message) { 'Created a new file with a very very looooooooooooooooooooooooooooooooooooooooooooooong commit message' } let(:invalid_c_params) do { branch: 'master', diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 49eea2e362b..606fa9185d8 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -20,7 +20,7 @@ describe API::Labels do create(:labeled_merge_request, labels: [priority_label], author: user, source_project: project ) expected_keys = %w( - id name color description + id name color text_color description open_issues_count closed_issues_count open_merge_requests_count subscribed priority ) @@ -43,6 +43,7 @@ describe API::Labels do expect(label1_response['open_merge_requests_count']).to eq(0) expect(label1_response['name']).to eq(label1.name) expect(label1_response['color']).to be_present + expect(label1_response['text_color']).to be_present expect(label1_response['description']).to be_nil expect(label1_response['priority']).to be_nil expect(label1_response['subscribed']).to be_falsey @@ -52,6 +53,7 @@ describe API::Labels do expect(group_label_response['open_merge_requests_count']).to eq(0) expect(group_label_response['name']).to eq(group_label.name) expect(group_label_response['color']).to be_present + expect(group_label_response['text_color']).to be_present expect(group_label_response['description']).to be_nil expect(group_label_response['priority']).to be_nil expect(group_label_response['subscribed']).to be_falsey @@ -61,6 +63,7 @@ describe API::Labels do expect(priority_label_response['open_merge_requests_count']).to eq(1) expect(priority_label_response['name']).to eq(priority_label.name) expect(priority_label_response['color']).to be_present + expect(priority_label_response['text_color']).to be_present expect(priority_label_response['description']).to be_nil expect(priority_label_response['priority']).to eq(3) expect(priority_label_response['subscribed']).to be_falsey diff --git a/spec/requests/api/wikis_spec.rb b/spec/requests/api/wikis_spec.rb index 6109829aad1..d1b58aac104 100644 --- a/spec/requests/api/wikis_spec.rb +++ b/spec/requests/api/wikis_spec.rb @@ -100,6 +100,8 @@ describe API::Wikis do shared_examples_for 'updates wiki page' do it 'updates the wiki page' do + put(api(url, user), params: payload) + expect(response).to have_gitlab_http_status(200) expect(json_response.size).to eq(4) expect(json_response.keys).to match_array(expected_keys_with_content) @@ -107,6 +109,16 @@ describe API::Wikis do expect(json_response['slug']).to eq(payload[:title].tr(' ', '-')) expect(json_response['title']).to eq(payload[:title]) end + + [:title, :content, :format].each do |part| + it "it updates with wiki with missing #{part}" do + payload.delete(part) + + put(api(url, user), params: payload) + + expect(response).to have_gitlab_http_status(200) + end + end end shared_examples_for '403 Forbidden' do @@ -528,8 +540,6 @@ describe API::Wikis do context 'when user is developer' do before do project.add_developer(user) - - put(api(url, user), params: payload) end include_examples 'updates wiki page' @@ -537,6 +547,10 @@ describe API::Wikis do context 'when page is not existing' do let(:url) { "/projects/#{project.id}/wikis/unknown" } + before do + put(api(url, user), params: payload) + end + include_examples '404 Wiki Page Not Found' end end @@ -544,8 +558,6 @@ describe API::Wikis do context 'when user is maintainer' do before do project.add_maintainer(user) - - put(api(url, user), params: payload) end include_examples 'updates wiki page' @@ -553,6 +565,10 @@ describe API::Wikis do context 'when page is not existing' do let(:url) { "/projects/#{project.id}/wikis/unknown" } + before do + put(api(url, user), params: payload) + end + include_examples '404 Wiki Page Not Found' end end @@ -572,8 +588,6 @@ describe API::Wikis do context 'when user is developer' do before do project.add_developer(user) - - put(api(url, user), params: payload) end include_examples 'updates wiki page' @@ -581,6 +595,10 @@ describe API::Wikis do context 'when page is not existing' do let(:url) { "/projects/#{project.id}/wikis/unknown" } + before do + put(api(url, user), params: payload) + end + include_examples '404 Wiki Page Not Found' end end @@ -588,8 +606,6 @@ describe API::Wikis do context 'when user is maintainer' do before do project.add_maintainer(user) - - put(api(url, user), params: payload) end include_examples 'updates wiki page' @@ -597,6 +613,10 @@ describe API::Wikis do context 'when page is not existing' do let(:url) { "/projects/#{project.id}/wikis/unknown" } + before do + put(api(url, user), params: payload) + end + include_examples '404 Wiki Page Not Found' end end @@ -605,10 +625,6 @@ describe API::Wikis do context 'when wiki belongs to a group project' do let(:project) { create(:project, :wiki_repo, namespace: group) } - before do - put(api(url, user), params: payload) - end - include_examples 'updates wiki page' end end diff --git a/spec/support/api/milestones_shared_examples.rb b/spec/support/api/milestones_shared_examples.rb index 5f709831ce1..b426fadb001 100644 --- a/spec/support/api/milestones_shared_examples.rb +++ b/spec/support/api/milestones_shared_examples.rb @@ -8,12 +8,17 @@ shared_examples_for 'group and project milestones' do |route_definition| describe "GET #{route_definition}" do it 'returns milestones list' do + create(:issue, project: project, milestone: milestone) + create(:closed_issue, project: project, milestone: milestone) + create(:closed_issue, project: project, milestone: milestone) + get api(route, user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(json_response.first['title']).to eq(milestone.title) + expect(json_response.first['percentage_complete']).to eq(66) end it 'returns a 401 error if user not authenticated' do diff --git a/spec/support/helpers/stub_feature_flags.rb b/spec/support/helpers/stub_feature_flags.rb index 4061a8d1bc9..48258692304 100644 --- a/spec/support/helpers/stub_feature_flags.rb +++ b/spec/support/helpers/stub_feature_flags.rb @@ -1,11 +1,30 @@ module StubFeatureFlags # Stub Feature flags with `flag_name: true/false` # - # @param [Hash] features where key is feature name and value is boolean whether enabled or not + # @param [Hash] features where key is feature name and value is boolean whether enabled or not. + # Alternatively, you can specify Hash to enable the flag on a specific thing. + # + # Examples + # - `stub_feature_flags(ci_live_trace: false)` ... Disable `ci_live_trace` + # feature flag globally. + # - `stub_feature_flags(ci_live_trace: { enabled: false, thing: project })` ... + # Disable `ci_live_trace` feature flag on the specified project. def stub_feature_flags(features) - features.each do |feature_name, enabled| - allow(Feature).to receive(:enabled?).with(feature_name, any_args) { enabled } - allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args) { enabled } + features.each do |feature_name, option| + if option.is_a?(Hash) + enabled, thing = option.values_at(:enabled, :thing) + else + enabled = option + thing = nil + end + + if thing + allow(Feature).to receive(:enabled?).with(feature_name, thing, any_args) { enabled } + allow(Feature).to receive(:enabled?).with(feature_name.to_s, thing, any_args) { enabled } + else + allow(Feature).to receive(:enabled?).with(feature_name, any_args) { enabled } + allow(Feature).to receive(:enabled?).with(feature_name.to_s, any_args) { enabled } + end end end end |