From 0cb88374cea8d7b907b40ae223b34f5b946d0100 Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Wed, 9 Aug 2017 21:17:15 +1000 Subject: update gitignores and licenses - generated by bin/rake gitlab:update_templates --- vendor/gitignore/Global/JetBrains.gitignore | 2 +- vendor/gitignore/Haskell.gitignore | 1 + vendor/gitignore/Prestashop.gitignore | 4 ++- vendor/gitignore/Smalltalk.gitignore | 4 +++ vendor/gitignore/Symfony.gitignore | 3 ++ vendor/gitignore/VisualStudio.gitignore | 2 +- vendor/gitlab-ci-yml/Go.gitlab-ci.yml | 32 ++++++++++----------- vendor/gitlab-ci-yml/Gradle.gitlab-ci.yml | 43 +++++++++++++---------------- vendor/gitlab-ci-yml/Laravel.gitlab-ci.yml | 4 +++ vendor/gitlab-ci-yml/PHP.gitlab-ci.yml | 3 ++ vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml | 8 +++--- 11 files changed, 58 insertions(+), 48 deletions(-) diff --git a/vendor/gitignore/Global/JetBrains.gitignore b/vendor/gitignore/Global/JetBrains.gitignore index ff23445e2b0..345e61ae3f2 100644 --- a/vendor/gitignore/Global/JetBrains.gitignore +++ b/vendor/gitignore/Global/JetBrains.gitignore @@ -31,7 +31,7 @@ cmake-build-debug/ ## Plugin-specific files: # IntelliJ -/out/ +out/ # mpeltonen/sbt-idea plugin .idea_modules/ diff --git a/vendor/gitignore/Haskell.gitignore b/vendor/gitignore/Haskell.gitignore index 450f32ec40c..eee88b2f0f7 100644 --- a/vendor/gitignore/Haskell.gitignore +++ b/vendor/gitignore/Haskell.gitignore @@ -18,3 +18,4 @@ cabal.sandbox.config .stack-work/ cabal.project.local .HTF/ +.ghc.environment.* diff --git a/vendor/gitignore/Prestashop.gitignore b/vendor/gitignore/Prestashop.gitignore index 7c6ae1e31cc..81f45e19eba 100644 --- a/vendor/gitignore/Prestashop.gitignore +++ b/vendor/gitignore/Prestashop.gitignore @@ -7,8 +7,10 @@ config/settings.*.php # The following files are generated by PrestaShop. admin-dev/autoupgrade/ -/cache/ +/cache/* !/cache/index.php +!/cache/*/ +/cache/*/* !/cache/cachefs/index.php !/cache/purifier/index.php !/cache/push/index.php diff --git a/vendor/gitignore/Smalltalk.gitignore b/vendor/gitignore/Smalltalk.gitignore index 75272b23472..943995e1172 100644 --- a/vendor/gitignore/Smalltalk.gitignore +++ b/vendor/gitignore/Smalltalk.gitignore @@ -13,6 +13,10 @@ SqueakDebug.log # Monticello package cache /package-cache +# playground cache +/play-cache +/play-stash + # Metacello-github cache /github-cache github-*.zip diff --git a/vendor/gitignore/Symfony.gitignore b/vendor/gitignore/Symfony.gitignore index 6c224e024e9..85fd714a965 100644 --- a/vendor/gitignore/Symfony.gitignore +++ b/vendor/gitignore/Symfony.gitignore @@ -39,3 +39,6 @@ # Backup entities generated with doctrine:generate:entities command **/Entity/*~ + +# Embedded web-server pid file +/.web-server-pid diff --git a/vendor/gitignore/VisualStudio.gitignore b/vendor/gitignore/VisualStudio.gitignore index 22fd88a55a3..89c66054885 100644 --- a/vendor/gitignore/VisualStudio.gitignore +++ b/vendor/gitignore/VisualStudio.gitignore @@ -151,7 +151,7 @@ publish/ # Publish Web Output *.[Pp]ublish.xml *.azurePubxml -# TODO: Comment the next line if you want to checkin your web deploy settings +# Note: Comment the next line if you want to checkin your web deploy settings, # but database connection strings (with potential passwords) will be unencrypted *.pubxml *.publishproj diff --git a/vendor/gitlab-ci-yml/Go.gitlab-ci.yml b/vendor/gitlab-ci-yml/Go.gitlab-ci.yml index e23b6e212f0..8a214352d2a 100644 --- a/vendor/gitlab-ci-yml/Go.gitlab-ci.yml +++ b/vendor/gitlab-ci-yml/Go.gitlab-ci.yml @@ -1,14 +1,19 @@ image: golang:latest +variables: + # Please edit to your GitLab project + REPO_NAME: gitlab.com/namespace/project + # The problem is that to be able to use go get, one needs to put # the repository in the $GOPATH. So for example if your gitlab domain -# is mydomainperso.com, and that your repository is repos/projectname, and +# is gitlab.com, and that your repository is namespace/project, and # the default GOPATH being /go, then you'd need to have your -# repository in /go/src/mydomainperso.com/repos/projectname +# repository in /go/src/gitlab.com/namespace/project # Thus, making a symbolic link corrects this. before_script: - - ln -s /builds /go/src/mydomainperso.com - - cd /go/src/mydomainperso.com/repos/projectname + - mkdir -p $GOPATH/src/$REPO_NAME + - ln -svf $CI_PROJECT_DIR/* $GOPATH/src/$REPO_NAME + - cd $GOPATH/src/$REPO_NAME stages: - test @@ -17,21 +22,14 @@ stages: format: stage: test script: - # Add here all the dependencies, or use glide/govendor to get - # them automatically. - # - curl https://glide.sh/get | sh - - go get github.com/alecthomas/kingpin - - go tool vet -composites=false -shadow=true *.go - - go test -race $(go list ./... | grep -v /vendor/) + - go fmt $(go list ./... | grep -v /vendor/) + - go vet $(go list ./... | grep -v /vendor/) + - go test -race $(go list ./... | grep -v /vendor/) compile: stage: build script: - # Add here all the dependencies, or use glide/govendor/... - # to get them automatically. - - go get github.com/alecthomas/kingpin - # Better put this in a Makefile - - go build -race -ldflags "-extldflags '-static'" -o mybinary + - go build -race -ldflags "-extldflags '-static'" -o mybinary artifacts: - paths: - - mybinary + paths: + - mybinary diff --git a/vendor/gitlab-ci-yml/Gradle.gitlab-ci.yml b/vendor/gitlab-ci-yml/Gradle.gitlab-ci.yml index a65e48a3389..48d98dddfad 100644 --- a/vendor/gitlab-ci-yml/Gradle.gitlab-ci.yml +++ b/vendor/gitlab-ci-yml/Gradle.gitlab-ci.yml @@ -1,41 +1,36 @@ -# This template uses the java:8 docker image because there isn't any -# official Gradle image at this moment -# # This is the Gradle build system for JVM applications # https://gradle.org/ # https://github.com/gradle/gradle -image: java:8 +image: gradle:alpine # Disable the Gradle daemon for Continuous Integration servers as correctness # is usually a priority over speed in CI environments. Using a fresh # runtime for each build is more reliable since the runtime is completely # isolated from any previous builds. variables: - GRADLE_OPTS: "-Dorg.gradle.daemon=false" + GRADLE_OPTS: "-Dorg.gradle.daemon=false" -# Make the gradle wrapper executable. This essentially downloads a copy of -# Gradle to build the project with. -# https://docs.gradle.org/current/userguide/gradle_wrapper.html -# It is expected that any modern gradle project has a wrapper before_script: - - chmod +x gradlew + - export GRADLE_USER_HOME=`pwd`/.gradle -# We redirect the gradle user home using -g so that it caches the -# wrapper and dependencies. -# https://docs.gradle.org/current/userguide/gradle_command_line.html -# -# Unfortunately it also caches the build output so -# cleaning removes reminants of any cached builds. -# The assemble task actually builds the project. -# If it fails here, the tests can't run. build: stage: build - script: - - ./gradlew -g /cache/.gradle clean assemble - allow_failure: false + script: gradle --build-cache assemble + cache: + key: "$CI_COMMIT_REF_NAME" + policy: push + paths: + - build + - .gradle + -# Use the generated build output to run the tests. test: stage: test - script: - - ./gradlew -g /cache/.gradle check + script: gradle check + cache: + key: "$CI_COMMIT_REF_NAME" + policy: pull + paths: + - build + - .gradle + diff --git a/vendor/gitlab-ci-yml/Laravel.gitlab-ci.yml b/vendor/gitlab-ci-yml/Laravel.gitlab-ci.yml index 434de4f055a..0ad662cf704 100644 --- a/vendor/gitlab-ci-yml/Laravel.gitlab-ci.yml +++ b/vendor/gitlab-ci-yml/Laravel.gitlab-ci.yml @@ -34,6 +34,10 @@ before_script: # Install php extensions - docker-php-ext-install mbstring mcrypt pdo_mysql curl json intl gd xml zip bz2 opcache + # Install & enable Xdebug for code coverage reports + - pecl install xdebug + - docker-php-ext-enable xdebug + # Install Composer and project dependencies. - curl -sS https://getcomposer.org/installer | php - php composer.phar install diff --git a/vendor/gitlab-ci-yml/PHP.gitlab-ci.yml b/vendor/gitlab-ci-yml/PHP.gitlab-ci.yml index bb8caa49d6b..33f44ee9222 100644 --- a/vendor/gitlab-ci-yml/PHP.gitlab-ci.yml +++ b/vendor/gitlab-ci-yml/PHP.gitlab-ci.yml @@ -11,6 +11,9 @@ before_script: - apt-get install -yqq git libmcrypt-dev libpq-dev libcurl4-gnutls-dev libicu-dev libvpx-dev libjpeg-dev libpng-dev libxpm-dev zlib1g-dev libfreetype6-dev libxml2-dev libexpat1-dev libbz2-dev libgmp3-dev libldap2-dev unixodbc-dev libsqlite3-dev libaspell-dev libsnmp-dev libpcre3-dev libtidy-dev # Install PHP extensions - docker-php-ext-install mbstring mcrypt pdo_pgsql curl json intl gd xml zip bz2 opcache +# Install & enable Xdebug for code coverage reports +- pecl install xdebug +- docker-php-ext-enable xdebug # Install and run Composer - curl -sS https://getcomposer.org/installer | php - php composer.phar install diff --git a/vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml b/vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml index 4e181e85451..ff7bdd32239 100644 --- a/vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml +++ b/vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml @@ -1,6 +1,6 @@ # Official language image. Look for the different tagged releases at: # https://hub.docker.com/r/library/ruby/tags/ -image: "ruby:2.3" +image: "ruby:2.4" # Pick zero or more services to be used on all builds. # Only needed when using a docker container to run your tests in. @@ -40,9 +40,9 @@ rails: variables: DATABASE_URL: "postgresql://postgres:postgres@postgres:5432/$POSTGRES_DB" script: - - bundle exec rake db:migrate - - bundle exec rake db:seed - - bundle exec rake test + - rails db:migrate + - rails db:seed + - rails test # This deploy job uses a simple deploy flow to Heroku, other providers, e.g. AWS Elastic Beanstalk # are supported too: https://github.com/travis-ci/dpl -- cgit v1.2.1 From dbfe4c5d322b94ae738cee0b8925f71a9e3c3191 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 16 Aug 2017 12:28:18 +0200 Subject: Add badge for dependency status --- README.md | 1 + changelogs/unreleased/feature-dependency-status-badge.yml | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 changelogs/unreleased/feature-dependency-status-badge.yml diff --git a/README.md b/README.md index 9309922ae39..9ead6d51c5d 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Build status](https://gitlab.com/gitlab-org/gitlab-ce/badges/master/build.svg)](https://gitlab.com/gitlab-org/gitlab-ce/commits/master) [![Overall test coverage](https://gitlab.com/gitlab-org/gitlab-ce/badges/master/coverage.svg)](https://gitlab.com/gitlab-org/gitlab-ce/pipelines) +[![Dependency Status](https://gemnasium.com/gitlabhq/gitlabhq.svg)](https://gemnasium.com/gitlabhq/gitlabhq) [![Code Climate](https://codeclimate.com/github/gitlabhq/gitlabhq.svg)](https://codeclimate.com/github/gitlabhq/gitlabhq) [![Core Infrastructure Initiative Best Practices](https://bestpractices.coreinfrastructure.org/projects/42/badge)](https://bestpractices.coreinfrastructure.org/projects/42) [![Gitter](https://badges.gitter.im/gitlabhq/gitlabhq.svg)](https://gitter.im/gitlabhq/gitlabhq?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge) diff --git a/changelogs/unreleased/feature-dependency-status-badge.yml b/changelogs/unreleased/feature-dependency-status-badge.yml new file mode 100644 index 00000000000..1becff3585a --- /dev/null +++ b/changelogs/unreleased/feature-dependency-status-badge.yml @@ -0,0 +1,5 @@ +--- +title: Add badge for dependency status +merge_request: 13588 +author: Markus Koller +type: other -- cgit v1.2.1 From 94c24b89bd238a3af16e7887c4a1420462a8b0f0 Mon Sep 17 00:00:00 2001 From: winh Date: Thu, 10 Aug 2017 21:09:29 +0200 Subject: Make comment form dropdown style consistent --- app/assets/stylesheets/pages/note_form.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 9558924bbcb..9600120b24f 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -27,6 +27,8 @@ .new-note, .note-edit-form { .note-form-actions { + @include new-style-dropdown; + position: relative; margin: $gl-padding 0 0; } -- cgit v1.2.1 From 59e5393827e9e9eddb9bb0a960f1cda1f6d9511d Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 23 Aug 2017 19:54:14 +0900 Subject: Fuzzy search issuable title or description --- app/models/concerns/issuable.rb | 11 ++-- lib/gitlab/sql/pattern.rb | 23 +++++++ spec/lib/gitlab/sql/pattern_spec.rb | 120 ++++++++++++++++++++++++++++++++++ spec/models/concerns/issuable_spec.rb | 42 ++++++++---- 4 files changed, 181 insertions(+), 15 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 3731b7c8577..681c3241dbb 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -6,6 +6,7 @@ # module Issuable extend ActiveSupport::Concern + include Gitlab::SQL::Pattern include CacheMarkdownField include Participable include Mentionable @@ -122,7 +123,9 @@ module Issuable # # Returns an ActiveRecord::Relation. def search(query) - where(arel_table[:title].matches("%#{query}%")) + title = to_fuzzy_arel(:title, query) + + where(title) end # Searches for records with a matching title or description. @@ -133,10 +136,10 @@ module Issuable # # Returns an ActiveRecord::Relation. def full_search(query) - t = arel_table - pattern = "%#{query}%" + title = to_fuzzy_arel(:title, query) + description = to_fuzzy_arel(:description, query) - where(t[:title].matches(pattern).or(t[:description].matches(pattern))) + where(title&.or(description)) end def sort(method, excluded_labels: []) diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index b42bc67ccfc..7c2d1d8f887 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -4,6 +4,7 @@ module Gitlab extend ActiveSupport::Concern MIN_CHARS_FOR_PARTIAL_MATCHING = 3 + REGEX_QUOTED_WORD = /(?<=^| )"[^"]+"(?= |$)/ class_methods do def to_pattern(query) @@ -17,6 +18,28 @@ module Gitlab def partial_matching?(query) query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end + + def to_fuzzy_arel(column, query) + words = select_fuzzy_words(query) + + matches = words.map { |word| arel_table[column].matches(to_pattern(word)) } + + matches.reduce { |result, match| result.and(match) } + end + + def select_fuzzy_words(query) + quoted_words = query.scan(REGEX_QUOTED_WORD) + + query = quoted_words.reduce(query) { |q, quoted_word| q.sub(quoted_word, '') } + + words = query.split(/\s+/) + + quoted_words.map! { |quoted_word| quoted_word[1..-2] } + + words.concat(quoted_words) + + words.select { |word| partial_matching?(word) } + end end end end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index 9d7b2136dab..0b0e905483f 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -52,4 +52,124 @@ describe Gitlab::SQL::Pattern do end end end + + describe '.select_fuzzy_words' do + subject(:select_fuzzy_words) { Issue.select_fuzzy_words(query) } + + context 'with a word equal to 3 chars' do + let(:query) { 'foo' } + + it 'returns array cotaining a word' do + expect(select_fuzzy_words).to match_array(['foo']) + end + end + + context 'with a word shorter than 3 chars' do + let(:query) { 'fo' } + + it 'returns empty array' do + expect(select_fuzzy_words).to match_array([]) + end + end + + context 'with two words both equal to 3 chars' do + let(:query) { 'foo baz' } + + it 'returns array containing two words' do + expect(select_fuzzy_words).to match_array(%w[foo baz]) + end + end + + context 'with two words divided by two speces both equal to 3 chars' do + let(:query) { 'foo baz' } + + it 'returns array containing two words' do + expect(select_fuzzy_words).to match_array(%w[foo baz]) + end + end + + context 'with two words equal to 3 chars and shorter than 3 chars' do + let(:query) { 'foo ba' } + + it 'returns array containing a word' do + expect(select_fuzzy_words).to match_array(['foo']) + end + end + + context 'with a multi-word surrounded by double quote' do + let(:query) { '"really bar"' } + + it 'returns array containing a multi-word' do + expect(select_fuzzy_words).to match_array(['really bar']) + end + end + + context 'with a multi-word surrounded by double quote and two words' do + let(:query) { 'foo "really bar" baz' } + + it 'returns array containing a multi-word and tow words' do + expect(select_fuzzy_words).to match_array(['foo', 'really bar', 'baz']) + end + end + + context 'with a multi-word surrounded by double quote missing a spece before the first double quote' do + let(:query) { 'foo"really bar"' } + + it 'returns array containing two words with double quote' do + expect(select_fuzzy_words).to match_array(['foo"really', 'bar"']) + end + end + + context 'with a multi-word surrounded by double quote missing a spece after the second double quote' do + let(:query) { '"really bar"baz' } + + it 'returns array containing two words with double quote' do + expect(select_fuzzy_words).to match_array(['"really', 'bar"baz']) + end + end + + context 'with two multi-word surrounded by double quote and two words' do + let(:query) { 'foo "really bar" baz "awesome feature"' } + + it 'returns array containing two multi-words and tow words' do + expect(select_fuzzy_words).to match_array(['foo', 'really bar', 'baz', 'awesome feature']) + end + end + end + + describe '.to_fuzzy_arel' do + subject(:to_fuzzy_arel) { Issue.to_fuzzy_arel(:title, query) } + + context 'with a word equal to 3 chars' do + let(:query) { 'foo' } + + it 'returns a single ILIKE condition' do + expect(to_fuzzy_arel.to_sql).to eq(%("issues"."title" ILIKE '%foo%')) + end + end + + context 'with a word shorter than 3 chars' do + let(:query) { 'fo' } + + it 'returns nil' do + expect(to_fuzzy_arel).to be_nil + end + end + + context 'with two words both equal to 3 chars' do + let(:query) { 'foo baz' } + + it 'returns a joining ILIKE condition using a AND' do + expect(to_fuzzy_arel.to_sql).to eq(%("issues"."title" ILIKE '%foo%' AND "issues"."title" ILIKE '%baz%')) + end + end + + context 'with a multi-word surrounded by double quote and two words' do + let(:query) { 'foo "really bar" baz' } + + it 'returns a joining ILIKE condition using a AND' do + expect(to_fuzzy_arel.to_sql).to eq(%("issues"."title" ILIKE '%foo%' AND "issues"."title" ILIKE '%baz%' AND "issues"."title" ILIKE '%really bar%')) + end + end + end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index dfbe1a7c192..37f6fd3a25b 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -66,56 +66,76 @@ describe Issuable do end describe ".search" do - let!(:searchable_issue) { create(:issue, title: "Searchable issue") } + let!(:searchable_issue) { create(:issue, title: "Searchable awesome issue") } - it 'returns notes with a matching title' do + it 'returns issues with a matching title' do expect(issuable_class.search(searchable_issue.title)) .to eq([searchable_issue]) end - it 'returns notes with a partially matching title' do + it 'returns issues with a partially matching title' do expect(issuable_class.search('able')).to eq([searchable_issue]) end - it 'returns notes with a matching title regardless of the casing' do + it 'returns issues with a matching title regardless of the casing' do expect(issuable_class.search(searchable_issue.title.upcase)) .to eq([searchable_issue]) end + + it 'returns issues with a fuzzy matching title' do + expect(issuable_class.search('searchable issue')).to eq([searchable_issue]) + end + + it 'returns all issues with a query shorter than 3 chars' do + expect(issuable_class.search('zz')).to eq(issuable_class.all) + end end describe ".full_search" do let!(:searchable_issue) do - create(:issue, title: "Searchable issue", description: 'kittens') + create(:issue, title: "Searchable awesome issue", description: 'Many cute kittens') end - it 'returns notes with a matching title' do + it 'returns issues with a matching title' do expect(issuable_class.full_search(searchable_issue.title)) .to eq([searchable_issue]) end - it 'returns notes with a partially matching title' do + it 'returns issues with a partially matching title' do expect(issuable_class.full_search('able')).to eq([searchable_issue]) end - it 'returns notes with a matching title regardless of the casing' do + it 'returns issues with a matching title regardless of the casing' do expect(issuable_class.full_search(searchable_issue.title.upcase)) .to eq([searchable_issue]) end - it 'returns notes with a matching description' do + it 'returns issues with a fuzzy matching title' do + expect(issuable_class.full_search('searchable issue')).to eq([searchable_issue]) + end + + it 'returns issues with a matching description' do expect(issuable_class.full_search(searchable_issue.description)) .to eq([searchable_issue]) end - it 'returns notes with a partially matching description' do + it 'returns issues with a partially matching description' do expect(issuable_class.full_search(searchable_issue.description)) .to eq([searchable_issue]) end - it 'returns notes with a matching description regardless of the casing' do + it 'returns issues with a matching description regardless of the casing' do expect(issuable_class.full_search(searchable_issue.description.upcase)) .to eq([searchable_issue]) end + + it 'returns issues with a fuzzy matching description' do + expect(issuable_class.full_search('many kittens')).to eq([searchable_issue]) + end + + it 'returns all issues with a query shorter than 3 chars' do + expect(issuable_class.search('zz')).to eq(issuable_class.all) + end end describe '.to_ability_name' do -- cgit v1.2.1 From c12ab72f05386903d1af78d0e766ee970af09a44 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 30 Aug 2017 23:37:23 +0900 Subject: Fix broken feature specs --- spec/features/boards/add_issues_modal_spec.rb | 4 ++-- spec/features/boards/boards_spec.rb | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/features/boards/add_issues_modal_spec.rb b/spec/features/boards/add_issues_modal_spec.rb index a6ad5981f8f..c480b5b7e34 100644 --- a/spec/features/boards/add_issues_modal_spec.rb +++ b/spec/features/boards/add_issues_modal_spec.rb @@ -8,8 +8,8 @@ describe 'Issue Boards add issue modal', :js do let!(:label) { create(:label, project: project) } let!(:list1) { create(:list, board: board, label: planning, position: 0) } let!(:list2) { create(:list, board: board, label: label, position: 1) } - let!(:issue) { create(:issue, project: project) } - let!(:issue2) { create(:issue, project: project) } + let!(:issue) { create(:issue, project: project, title: 'abc', description: 'def') } + let!(:issue2) { create(:issue, project: project, title: 'hij', description: 'klm') } before do project.team << [user, :master] diff --git a/spec/features/boards/boards_spec.rb b/spec/features/boards/boards_spec.rb index ce458431c55..c04b7ca89b3 100644 --- a/spec/features/boards/boards_spec.rb +++ b/spec/features/boards/boards_spec.rb @@ -71,15 +71,15 @@ describe 'Issue Boards', js: true do let!(:list2) { create(:list, board: board, label: development, position: 1) } let!(:confidential_issue) { create(:labeled_issue, :confidential, project: project, author: user, labels: [planning], relative_position: 9) } - let!(:issue1) { create(:labeled_issue, project: project, assignees: [user], labels: [planning], relative_position: 8) } - let!(:issue2) { create(:labeled_issue, project: project, author: user2, labels: [planning], relative_position: 7) } - let!(:issue3) { create(:labeled_issue, project: project, labels: [planning], relative_position: 6) } - let!(:issue4) { create(:labeled_issue, project: project, labels: [planning], relative_position: 5) } - let!(:issue5) { create(:labeled_issue, project: project, labels: [planning], milestone: milestone, relative_position: 4) } - let!(:issue6) { create(:labeled_issue, project: project, labels: [planning, development], relative_position: 3) } - let!(:issue7) { create(:labeled_issue, project: project, labels: [development], relative_position: 2) } - let!(:issue8) { create(:closed_issue, project: project) } - let!(:issue9) { create(:labeled_issue, project: project, labels: [planning, testing, bug, accepting], relative_position: 1) } + let!(:issue1) { create(:labeled_issue, project: project, title: 'aaa', description: '111', assignees: [user], labels: [planning], relative_position: 8) } + let!(:issue2) { create(:labeled_issue, project: project, title: 'bbb', description: '222', author: user2, labels: [planning], relative_position: 7) } + let!(:issue3) { create(:labeled_issue, project: project, title: 'ccc', description: '333', labels: [planning], relative_position: 6) } + let!(:issue4) { create(:labeled_issue, project: project, title: 'ddd', description: '444', labels: [planning], relative_position: 5) } + let!(:issue5) { create(:labeled_issue, project: project, title: 'eee', description: '555', labels: [planning], milestone: milestone, relative_position: 4) } + let!(:issue6) { create(:labeled_issue, project: project, title: 'fff', description: '666', labels: [planning, development], relative_position: 3) } + let!(:issue7) { create(:labeled_issue, project: project, title: 'ggg', description: '777', labels: [development], relative_position: 2) } + let!(:issue8) { create(:closed_issue, project: project, title: 'hhh', description: '888') } + let!(:issue9) { create(:labeled_issue, project: project, title: 'iii', description: '999', labels: [planning, testing, bug, accepting], relative_position: 1) } before do visit project_board_path(project, board) -- cgit v1.2.1 From bd82e61f910ce36a1c3ece4a680706db2cf05a2d Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Fri, 28 Jul 2017 10:27:27 -0500 Subject: Added monitoring_paths component to show multiple time series on a single chart --- .../monitoring/components/monitoring_paths.vue | 40 ++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 app/assets/javascripts/monitoring/components/monitoring_paths.vue diff --git a/app/assets/javascripts/monitoring/components/monitoring_paths.vue b/app/assets/javascripts/monitoring/components/monitoring_paths.vue new file mode 100644 index 00000000000..2d17f544cb6 --- /dev/null +++ b/app/assets/javascripts/monitoring/components/monitoring_paths.vue @@ -0,0 +1,40 @@ + + -- cgit v1.2.1 From 4ea9c9b687719012f43bf42406095f5137c18cec Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Fri, 28 Jul 2017 10:32:02 -0500 Subject: Converted the dates for each time series on the store --- .../monitoring/stores/monitoring_store.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/monitoring/stores/monitoring_store.js b/app/assets/javascripts/monitoring/stores/monitoring_store.js index 737c964f12e..541c8e7dd46 100644 --- a/app/assets/javascripts/monitoring/stores/monitoring_store.js +++ b/app/assets/javascripts/monitoring/stores/monitoring_store.js @@ -13,13 +13,21 @@ class MonitoringStore { let metricsRow = []; let index = 1; Object.keys(currentMetrics).forEach((key) => { - const metricValues = currentMetrics[key].queries[0].result[0].values; + const metricValues = currentMetrics[key].queries[0].result; if (metricValues != null) { - const literalMetrics = metricValues.map(metric => ({ - time: new Date(metric[0] * 1000), - value: metric[1], - })); - currentMetrics[key].queries[0].result[0].values = literalMetrics; + currentMetrics[key].queries[0].result = metricValues.map((series) => { + let convertedValues = []; + if (series != null) { + convertedValues = series.values.map(metric => ({ + time: new Date(metric[0] * 1000), + value: metric[1], + })); + } + return { + metric: series.metric, + values: convertedValues, + }; + }); metricsRow.push(currentMetrics[key]); if (index % 2 === 0) { availableMetrics.push(metricsRow); -- cgit v1.2.1 From faf5363ea01cc3dd56ff9b8f9b1061feff1eb3f0 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Fri, 28 Jul 2017 12:21:09 -0500 Subject: Refactored the monitoring_column component to process all of the time series --- .../javascripts/monitoring/components/graph.vue | 122 +++++++++++++-------- 1 file changed, 77 insertions(+), 45 deletions(-) diff --git a/app/assets/javascripts/monitoring/components/graph.vue b/app/assets/javascripts/monitoring/components/graph.vue index 6f6da9e1463..e577ded0423 100644 --- a/app/assets/javascripts/monitoring/components/graph.vue +++ b/app/assets/javascripts/monitoring/components/graph.vue @@ -3,10 +3,10 @@ import GraphLegend from './graph/legend.vue'; import GraphFlag from './graph/flag.vue'; import GraphDeployment from './graph/deployment.vue'; + import monitoringPaths from './monitoring_paths.vue'; import MonitoringMixin from '../mixins/monitoring_mixins'; import eventHub from '../event_hub'; import measurements from '../utils/measurements'; - import { formatRelevantDigits } from '../../lib/utils/number_utils'; import { timeScaleFormat } from '../utils/date_time_formatters'; import bp from '../../breakpoints'; @@ -39,36 +39,40 @@ graphHeight: 450, graphWidth: 600, graphHeightOffset: 120, - xScale: {}, - yScale: {}, margin: {}, - data: [], + breakpointHandler: Breakpoints.get(), unitOfDisplay: '', areaColorRgb: '#8fbce8', lineColorRgb: '#1f78d1', yAxisLabel: '', legendTitle: '', reducedDeploymentData: [], - area: '', - line: '', measurements: measurements.large, currentData: { time: new Date(), value: 0, }, - currentYCoordinate: 0, + currentDataIndex: 0, currentXCoordinate: 0, currentFlagPosition: 0, metricUsage: '', showFlag: false, showDeployInfo: true, + timeSeries: [], }; }, components: { +<<<<<<< HEAD:app/assets/javascripts/monitoring/components/graph.vue GraphLegend, GraphFlag, GraphDeployment, +======= + monitoringLegends, + monitoringFlag, + monitoringDeployment, + monitoringPaths, +>>>>>>> Refactored the monitoring_column component to process all of the time series:app/assets/javascripts/monitoring/components/monitoring_column.vue }, computed: { @@ -104,17 +108,14 @@ this.margin = measurements.small.margin; this.measurements = measurements.small; } - this.data = query.result[0].values; this.unitOfDisplay = query.unit || ''; this.yAxisLabel = this.graphData.y_label || 'Values'; this.legendTitle = query.label || 'Average'; this.graphWidth = this.$refs.baseSvg.clientWidth - this.margin.left - this.margin.right; this.graphHeight = this.graphHeight - this.margin.top - this.margin.bottom; - if (this.data !== undefined) { - this.renderAxesPaths(); - this.formatDeployments(); - } + this.renderAxesPaths(); + this.formatDeployments(); }, handleMouseOverGraph(e) { @@ -123,16 +124,17 @@ point.y = e.clientY; point = point.matrixTransform(this.$refs.graphData.getScreenCTM().inverse()); point.x = point.x += 7; - const timeValueOverlay = this.xScale.invert(point.x); - const overlayIndex = bisectDate(this.data, timeValueOverlay, 1); - const d0 = this.data[overlayIndex - 1]; - const d1 = this.data[overlayIndex]; + const firstTimeSeries = this.timeSeries[0]; + const timeValueOverlay = firstTimeSeries.timeSeriesScaleX.invert(point.x); + const overlayIndex = bisectDate(firstTimeSeries.values, timeValueOverlay, 1); + const d0 = firstTimeSeries.values[overlayIndex - 1]; + const d1 = firstTimeSeries.values[overlayIndex]; if (d0 === undefined || d1 === undefined) return; const evalTime = timeValueOverlay - d0[0] > d1[0] - timeValueOverlay; this.currentData = evalTime ? d1 : d0; - this.currentXCoordinate = Math.floor(this.xScale(this.currentData.time)); + this.currentDataIndex = evalTime ? overlayIndex : (overlayIndex - 1); + this.currentXCoordinate = Math.floor(firstTimeSeries.timeSeriesScaleX(this.currentData.time)); const currentDeployXPos = this.mouseOverDeployInfo(point.x); - this.currentYCoordinate = this.yScale(this.currentData.value); if (this.currentXCoordinate > (this.graphWidth - 200)) { this.currentFlagPosition = this.currentXCoordinate - 103; @@ -145,17 +147,45 @@ } else { this.showFlag = true; } - - this.metricUsage = `${formatRelevantDigits(this.currentData.value)} ${this.unitOfDisplay}`; }, renderAxesPaths() { + this.timeSeries = this.columnData.queries[0].result.map((timeSeries) => { + const timeSeriesScaleX = d3.time.scale() + .range([0, this.graphWidth - 70]); + + const timeSeriesScaleY = d3.scale.linear() + .range([this.graphHeight - this.graphHeightOffset, 0]); + + timeSeriesScaleX.domain(d3.extent(timeSeries.values, d => d.time)); + timeSeriesScaleY.domain([0, d3.max(timeSeries.values.map(d => d.value))]); + + const lineFunction = d3.svg.line() + .x(d => timeSeriesScaleX(d.time)) + .y(d => timeSeriesScaleY(d.value)); + + const areaFunction = d3.svg.area() + .x(d => timeSeriesScaleX(d.time)) + .y0(this.graphHeight - this.graphHeightOffset) + .y1(d => timeSeriesScaleY(d.value)) + .interpolate('linear'); + + return { + linePath: lineFunction(timeSeries.values), + areaPath: areaFunction(timeSeries.values), + timeSeriesScaleX, + timeSeriesScaleY, + values: timeSeries.values, + }; + }); + const axisXScale = d3.time.scale() .range([0, this.graphWidth]); - this.yScale = d3.scale.linear() + const axisYScale = d3.scale.linear() .range([this.graphHeight - this.graphHeightOffset, 0]); - axisXScale.domain(d3.extent(this.data, d => d.time)); - this.yScale.domain([0, d3.max(this.data.map(d => d.value))]); + + axisXScale.domain(d3.extent(this.timeSeries[0].values, d => d.time)); + axisYScale.domain([0, d3.max(this.timeSeries[0].values.map(d => d.value))]); const xAxis = d3.svg.axis() .scale(axisXScale) @@ -164,7 +194,7 @@ .orient('bottom'); const yAxis = d3.svg.axis() - .scale(this.yScale) + .scale(axisYScale) .ticks(measurements.yTicks) .orient('left'); @@ -180,25 +210,6 @@ .attr('class', 'axis-tick'); } // Avoid adding the class to the first tick, to prevent coloring }); // This will select all of the ticks once they're rendered - - this.xScale = d3.time.scale() - .range([0, this.graphWidth - 70]); - - this.xScale.domain(d3.extent(this.data, d => d.time)); - - const areaFunction = d3.svg.area() - .x(d => this.xScale(d.time)) - .y0(this.graphHeight - this.graphHeightOffset) - .y1(d => this.yScale(d.value)) - .interpolate('linear'); - - const lineFunction = d3.svg.line() - .x(d => this.xScale(d.time)) - .y(d => this.yScale(d.value)); - - this.line = lineFunction(this.data); - - this.area = areaFunction(this.data); }, }, @@ -248,12 +259,15 @@ :area-color-rgb="areaColorRgb" :legend-title="legendTitle" :y-axis-label="yAxisLabel" - :metric-usage="metricUsage" + :time-series="timeSeries" + :unit-of-display="unitOfDisplay" + :current-data-index="currentDataIndex" /> +<<<<<<< HEAD:app/assets/javascripts/monitoring/components/graph.vue + + + >>>>>> Refactored the monitoring_column component to process all of the time series:app/assets/javascripts/monitoring/components/monitoring_column.vue :show-deploy-info="showDeployInfo" :deployment-data="reducedDeploymentData" :graph-height="graphHeight" @@ -277,7 +310,6 @@ Date: Fri, 28 Jul 2017 12:23:53 -0500 Subject: Added legends for each of the time series --- .../monitoring/components/graph/flag.vue | 15 +---- .../monitoring/components/graph/legend.vue | 71 ++++++++++++++++------ 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/app/assets/javascripts/monitoring/components/graph/flag.vue b/app/assets/javascripts/monitoring/components/graph/flag.vue index c4d4647d240..a98e3d06c18 100644 --- a/app/assets/javascripts/monitoring/components/graph/flag.vue +++ b/app/assets/javascripts/monitoring/components/graph/flag.vue @@ -7,10 +7,6 @@ type: Number, required: true, }, - currentYCoordinate: { - type: Number, - required: true, - }, currentFlagPosition: { type: Number, required: true, @@ -60,16 +56,7 @@ :y2="calculatedHeight" transform="translate(-5, 20)"> - - - diff --git a/app/assets/javascripts/monitoring/components/graph/legend.vue b/app/assets/javascripts/monitoring/components/graph/legend.vue index d08f9cbffd4..088612cadd3 100644 --- a/app/assets/javascripts/monitoring/components/graph/legend.vue +++ b/app/assets/javascripts/monitoring/components/graph/legend.vue @@ -1,4 +1,6 @@ +
  • diff --git a/changelogs/unreleased/37331-button-MR-widget.yml b/changelogs/unreleased/37331-button-MR-widget.yml new file mode 100644 index 00000000000..59bc1bd201e --- /dev/null +++ b/changelogs/unreleased/37331-button-MR-widget.yml @@ -0,0 +1,5 @@ +--- +title: Fix buttons with different height in merge request widget +merge_request: +author: +type: fixed -- cgit v1.2.1 From bfc9b3b1b3a9184a1308eed131cfcd20226df710 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Fri, 1 Sep 2017 12:22:15 -0500 Subject: Corrected collate function --- app/assets/javascripts/monitoring/stores/monitoring_store.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/monitoring/stores/monitoring_store.js b/app/assets/javascripts/monitoring/stores/monitoring_store.js index 5dc414158fd..0a4cdd88044 100644 --- a/app/assets/javascripts/monitoring/stores/monitoring_store.js +++ b/app/assets/javascripts/monitoring/stores/monitoring_store.js @@ -25,7 +25,7 @@ function collate(array, rows = 2) { let row = []; array.forEach((value, index) => { row.push(value); - if (index % rows === 0) { + if ((index + 1) % rows === 0) { collatedArray.push(row); row = []; } -- cgit v1.2.1 From 9994f599abac0da450ab5c05a580973faaca9675 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sun, 3 Sep 2017 12:58:56 +0900 Subject: Add `Searching for specific terms` section [ci skip] --- doc/user/search/index.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/user/search/index.md b/doc/user/search/index.md index f5c7ce49e8e..609d66d1dcb 100644 --- a/doc/user/search/index.md +++ b/doc/user/search/index.md @@ -40,6 +40,18 @@ The same process is valid for merge requests. Navigate to your project's **Merge and click **Search or filter results...**. Merge requests can be filtered by author, assignee, milestone, and label. +### Searching for specific terms + +You can filter issues by specific terms included in titles or descriptions. + +* Syntax + * Searches look for all the words in a query, in any order - e.g.: searching + issues for `display bug` will return all issues matching both those words, in any order. + * To find the exact term, use double quotes: `"display bug"` +* Limitation + * For a performance reason, the terms shorter than 3 chars are ignored - e.g.: searching + issues for `included in titles` is same with `included titles` + ### Issues and merge requests per group Similar to **Issues and merge requests per project**, you can also search for issues -- cgit v1.2.1 From 8ca5c333fd5170a900c7fa28b6bfcbe1a8bc6477 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 18 Aug 2017 17:25:35 +0900 Subject: Extend API: Pipeline Schedule Variable --- doc/api/pipeline_schedules.md | 161 ++++++++++++++++++++++- lib/api/entities.rb | 3 +- lib/api/pipeline_schedules.rb | 70 ++++++++++ spec/fixtures/api/schemas/pipeline_schedule.json | 11 ++ 4 files changed, 243 insertions(+), 2 deletions(-) diff --git a/doc/api/pipeline_schedules.md b/doc/api/pipeline_schedules.md index 433654c18cc..d2169b0377d 100644 --- a/doc/api/pipeline_schedules.md +++ b/doc/api/pipeline_schedules.md @@ -84,7 +84,13 @@ curl --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gitlab.example.com/ "state": "active", "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "web_url": "https://gitlab.example.com/root" - } + }, + "variables": [ + { + "key": "TEST_VARIABLE_1", + "value": "TEST_1" + } + ] } ``` @@ -271,3 +277,156 @@ curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gi } } ``` + +## Create a new pipeline schedule variable + +Create a new variable of a pipeline schedule. + +``` +POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables +``` + +| Attribute | Type | required | Description | +|---------------|---------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed | +| `value` | string | yes | The `value` of a variable | + +```sh +curl --request POST --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "key=NEW_VARIABLE" --form "value=new value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables" +``` + +```json +{ + "id": 13, + "description": "Test schedule pipeline", + "ref": "master", + "cron": "* * * * *", + "cron_timezone": "Asia/Tokyo", + "next_run_at": "2017-05-19T13:41:00.000Z", + "active": true, + "created_at": "2017-05-19T13:31:08.849Z", + "updated_at": "2017-05-19T13:40:17.727Z", + "last_pipeline": { + "id": 332, + "sha": "0e788619d0b5ec17388dffb973ecd505946156db", + "ref": "master", + "status": "pending" + }, + "owner": { + "name": "Administrator", + "username": "root", + "id": 1, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "web_url": "https://gitlab.example.com/root" + }, + "variables": [ + { + "key": "NEW_VARIABLE", + "value": "new value" + } + ] +} +``` + +## Edit a pipeline schedule variable + +Updates the variable of a pipeline schedule. + +``` +PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key +``` + +| Attribute | Type | required | Description | +|---------------|---------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable | +| `value` | string | yes | The `value` of a variable | + +```sh +curl --request PUT --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "value=updated value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE" +``` + +```json +{ + "id": 13, + "description": "Test schedule pipeline", + "ref": "master", + "cron": "* * * * *", + "cron_timezone": "Asia/Tokyo", + "next_run_at": "2017-05-19T13:41:00.000Z", + "active": true, + "created_at": "2017-05-19T13:31:08.849Z", + "updated_at": "2017-05-19T13:40:17.727Z", + "last_pipeline": { + "id": 332, + "sha": "0e788619d0b5ec17388dffb973ecd505946156db", + "ref": "master", + "status": "pending" + }, + "owner": { + "name": "Administrator", + "username": "root", + "id": 1, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "web_url": "https://gitlab.example.com/root" + }, + "variables": [ + { + "key": "NEW_VARIABLE", + "value": "updated value" + } + ] +} +``` + +## Delete a pipeline schedule variable + +Delete the variable of a pipeline schedule. + +``` +DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key +``` + +| Attribute | Type | required | Description | +|----------------|---------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable | + +```sh +curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE" +``` + +```json +{ + "id": 13, + "description": "Test schedule pipeline", + "ref": "master", + "cron": "* * * * *", + "cron_timezone": "Asia/Tokyo", + "next_run_at": "2017-05-19T13:41:00.000Z", + "active": true, + "created_at": "2017-05-19T13:31:08.849Z", + "updated_at": "2017-05-19T13:40:17.727Z", + "last_pipeline": { + "id": 332, + "sha": "0e788619d0b5ec17388dffb973ecd505946156db", + "ref": "master", + "status": "pending" + }, + "owner": { + "name": "Administrator", + "username": "root", + "id": 1, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "web_url": "https://gitlab.example.com/root" + }, + "variables": [] +} +``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f13f2d723bb..09d7d9ad349 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -819,7 +819,7 @@ module API class Variable < Grape::Entity expose :key, :value - expose :protected?, as: :protected + expose :protected?, as: :protected, if: -> (entity, options) { entity.respond_to?(protected?) } end class Pipeline < PipelineBasic @@ -840,6 +840,7 @@ module API class PipelineScheduleDetails < PipelineSchedule expose :last_pipeline, using: Entities::PipelineBasic + expose :variables, using: Entities::Variable end class EnvironmentBasic < Grape::Entity diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index ef01cbc7875..e82b974c8cd 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -119,6 +119,76 @@ module API destroy_conditionally!(pipeline_schedule) end + + params do + requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' + end + resource :variables, requirements: { pipeline_schedule_id: %r{[^/]+} } do + desc 'Create a new pipeline schedule variable' do + success Entities::PipelineScheduleDetails + end + params do + requires :key, type: String, desc: 'The key of the variable' + requires :value, type: String, desc: 'The value of the variable' + end + post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do + authorize! :read_pipeline_schedule, user_project + + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :update_pipeline_schedule, pipeline_schedule + + variable_params = declared_params(include_missing: false) + variable = pipeline_schedule.variables.create(variable_params) + + if variable.persisted? + present variable, with: Entities::Variable + else + render_validation_error!(variable) + end + end + + desc 'Edit a pipeline schedule variable' do + success Entities::PipelineScheduleDetails + end + params do + optional :key, type: String, desc: 'The key of the variable' + optional :value, type: String, desc: 'The value of the variable' + end + put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + authorize! :read_pipeline_schedule, user_project + + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :update_pipeline_schedule, pipeline_schedule + + variable = pipeline_schedule.variables.find_by(key: params[:key]) + not_found!('Variable') unless variable + + if variable.update(declared_params(include_missing: false)) + present variable, with: Entities::Variable + else + render_validation_error!(variable) + end + end + + desc 'Delete a pipeline schedule variable' do + success Entities::PipelineScheduleDetails + end + params do + requires :key, type: String, desc: 'The key of the variable' + end + delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + authorize! :read_pipeline_schedule, user_project + + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :admin_pipeline_schedule, pipeline_schedule + + variable = pipeline_schedule.variables.find_by(key: params[:key]) + not_found!('Variable') unless variable + + status :accepted + present variable, with: Entities::Variable + end + end end helpers do diff --git a/spec/fixtures/api/schemas/pipeline_schedule.json b/spec/fixtures/api/schemas/pipeline_schedule.json index f6346bd0fb6..16bbeacddbd 100644 --- a/spec/fixtures/api/schemas/pipeline_schedule.json +++ b/spec/fixtures/api/schemas/pipeline_schedule.json @@ -31,6 +31,17 @@ "web_url": { "type": "uri" } }, "additionalProperties": false + }, + "variables": { + "type": ["array", "null"], + "items": { + "type": ["object", "null"], + "properties": { + "key": { "type": "string" }, + "value": { "type": "string" } + }, + "additionalProperties": false + } } }, "required": [ -- cgit v1.2.1 From 683ba258cb95d5596b545d94420a9b6262f5d45b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 18 Aug 2017 17:46:14 +0900 Subject: Fix doc --- doc/api/pipeline_schedules.md | 95 +++++-------------------------------------- 1 file changed, 11 insertions(+), 84 deletions(-) diff --git a/doc/api/pipeline_schedules.md b/doc/api/pipeline_schedules.md index d2169b0377d..bfa6fb731eb 100644 --- a/doc/api/pipeline_schedules.md +++ b/doc/api/pipeline_schedules.md @@ -278,6 +278,10 @@ curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gi } ``` +## Pipeline schedule variable + +> [Introduced][ce-34518] in GitLab 10.0. + ## Create a new pipeline schedule variable Create a new variable of a pipeline schedule. @@ -299,35 +303,8 @@ curl --request POST --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "key=N ```json { - "id": 13, - "description": "Test schedule pipeline", - "ref": "master", - "cron": "* * * * *", - "cron_timezone": "Asia/Tokyo", - "next_run_at": "2017-05-19T13:41:00.000Z", - "active": true, - "created_at": "2017-05-19T13:31:08.849Z", - "updated_at": "2017-05-19T13:40:17.727Z", - "last_pipeline": { - "id": 332, - "sha": "0e788619d0b5ec17388dffb973ecd505946156db", - "ref": "master", - "status": "pending" - }, - "owner": { - "name": "Administrator", - "username": "root", - "id": 1, - "state": "active", - "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", - "web_url": "https://gitlab.example.com/root" - }, - "variables": [ - { - "key": "NEW_VARIABLE", - "value": "new value" - } - ] + "key": "NEW_VARIABLE", + "value": "new value" } ``` @@ -352,35 +329,8 @@ curl --request PUT --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "value= ```json { - "id": 13, - "description": "Test schedule pipeline", - "ref": "master", - "cron": "* * * * *", - "cron_timezone": "Asia/Tokyo", - "next_run_at": "2017-05-19T13:41:00.000Z", - "active": true, - "created_at": "2017-05-19T13:31:08.849Z", - "updated_at": "2017-05-19T13:40:17.727Z", - "last_pipeline": { - "id": 332, - "sha": "0e788619d0b5ec17388dffb973ecd505946156db", - "ref": "master", - "status": "pending" - }, - "owner": { - "name": "Administrator", - "username": "root", - "id": 1, - "state": "active", - "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", - "web_url": "https://gitlab.example.com/root" - }, - "variables": [ - { - "key": "NEW_VARIABLE", - "value": "updated value" - } - ] + "key": "NEW_VARIABLE", + "value": "updated value" } ``` @@ -403,30 +353,7 @@ curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gi ``` ```json -{ - "id": 13, - "description": "Test schedule pipeline", - "ref": "master", - "cron": "* * * * *", - "cron_timezone": "Asia/Tokyo", - "next_run_at": "2017-05-19T13:41:00.000Z", - "active": true, - "created_at": "2017-05-19T13:31:08.849Z", - "updated_at": "2017-05-19T13:40:17.727Z", - "last_pipeline": { - "id": 332, - "sha": "0e788619d0b5ec17388dffb973ecd505946156db", - "ref": "master", - "status": "pending" - }, - "owner": { - "name": "Administrator", - "username": "root", - "id": 1, - "state": "active", - "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", - "web_url": "https://gitlab.example.com/root" - }, - "variables": [] -} +// Empty ``` + +[ce-34518]: https://gitlab.com/gitlab-org/gitlab-ce/issues/34518 \ No newline at end of file -- cgit v1.2.1 From 278ac9ed5db6b191b42b52457492e551759f9d7d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 22 Aug 2017 01:23:17 +0900 Subject: Add changelog --- .../feature-sm-34518-extend-api-pipeline-schedule-variable-new.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-sm-34518-extend-api-pipeline-schedule-variable-new.yml diff --git a/changelogs/unreleased/feature-sm-34518-extend-api-pipeline-schedule-variable-new.yml b/changelogs/unreleased/feature-sm-34518-extend-api-pipeline-schedule-variable-new.yml new file mode 100644 index 00000000000..969a5aeaed3 --- /dev/null +++ b/changelogs/unreleased/feature-sm-34518-extend-api-pipeline-schedule-variable-new.yml @@ -0,0 +1,5 @@ +--- +title: 'Extend API: Pipeline Schedule Variable' +merge_request: 13653 +author: +type: added -- cgit v1.2.1 From 03f72f0f419b7e05fe9207c90b92b02ef7291cd1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 22 Aug 2017 02:21:37 +0900 Subject: Add spec (Halfway) --- lib/api/entities.rb | 2 +- lib/api/pipeline_schedules.rb | 103 +++++++------- spec/fixtures/api/schemas/pipeline_schedule.json | 9 +- .../api/schemas/pipeline_schedule_variable.json | 8 ++ spec/requests/api/pipeline_schedules_spec.rb | 156 +++++++++++++++++++++ 5 files changed, 216 insertions(+), 62 deletions(-) create mode 100644 spec/fixtures/api/schemas/pipeline_schedule_variable.json diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 09d7d9ad349..81cf7039260 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -819,7 +819,7 @@ module API class Variable < Grape::Entity expose :key, :value - expose :protected?, as: :protected, if: -> (entity, options) { entity.respond_to?(protected?) } + expose :protected?, as: :protected, if: -> (entity, options) { entity.respond_to?(:protected?) } end class Pipeline < PipelineBasic diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index e82b974c8cd..ffb9a09834d 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -120,74 +120,71 @@ module API destroy_conditionally!(pipeline_schedule) end + desc 'Create a new pipeline schedule variable' do + success Entities::Variable + end params do requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' + requires :key, type: String, desc: 'The key of the variable' + requires :value, type: String, desc: 'The value of the variable' end - resource :variables, requirements: { pipeline_schedule_id: %r{[^/]+} } do - desc 'Create a new pipeline schedule variable' do - success Entities::PipelineScheduleDetails - end - params do - requires :key, type: String, desc: 'The key of the variable' - requires :value, type: String, desc: 'The value of the variable' - end - post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do - authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule - authorize! :update_pipeline_schedule, pipeline_schedule + post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do + authorize! :read_pipeline_schedule, user_project - variable_params = declared_params(include_missing: false) - variable = pipeline_schedule.variables.create(variable_params) + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :update_pipeline_schedule, pipeline_schedule - if variable.persisted? - present variable, with: Entities::Variable - else - render_validation_error!(variable) - end + variable_params = declared_params(include_missing: false) + variable = pipeline_schedule.variables.create(variable_params) + if variable.persisted? + present variable, with: Entities::Variable + else + render_validation_error!(variable) end + end - desc 'Edit a pipeline schedule variable' do - success Entities::PipelineScheduleDetails - end - params do - optional :key, type: String, desc: 'The key of the variable' - optional :value, type: String, desc: 'The value of the variable' - end - put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do - authorize! :read_pipeline_schedule, user_project + desc 'Edit a pipeline schedule variable' do + success Entities::Variable + end + params do + requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' + requires :key, type: String, desc: 'The key of the variable' + optional :value, type: String, desc: 'The value of the variable' + end + put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + authorize! :read_pipeline_schedule, user_project - not_found!('PipelineSchedule') unless pipeline_schedule - authorize! :update_pipeline_schedule, pipeline_schedule + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :update_pipeline_schedule, pipeline_schedule - variable = pipeline_schedule.variables.find_by(key: params[:key]) - not_found!('Variable') unless variable + variable = pipeline_schedule.variables.find_by(key: params[:key]) + not_found!('Variable') unless variable - if variable.update(declared_params(include_missing: false)) - present variable, with: Entities::Variable - else - render_validation_error!(variable) - end + if variable.update(declared_params(include_missing: false)) + present variable, with: Entities::Variable + else + render_validation_error!(variable) end + end - desc 'Delete a pipeline schedule variable' do - success Entities::PipelineScheduleDetails - end - params do - requires :key, type: String, desc: 'The key of the variable' - end - delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do - authorize! :read_pipeline_schedule, user_project + desc 'Delete a pipeline schedule variable' do + success Entities::Variable + end + params do + requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' + requires :key, type: String, desc: 'The key of the variable' + end + delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + authorize! :read_pipeline_schedule, user_project - not_found!('PipelineSchedule') unless pipeline_schedule - authorize! :admin_pipeline_schedule, pipeline_schedule + not_found!('PipelineSchedule') unless pipeline_schedule + authorize! :admin_pipeline_schedule, pipeline_schedule - variable = pipeline_schedule.variables.find_by(key: params[:key]) - not_found!('Variable') unless variable + variable = pipeline_schedule.variables.find_by(key: params[:key]) + not_found!('Variable') unless variable - status :accepted - present variable, with: Entities::Variable - end + status :accepted + present variable, with: Entities::Variable end end diff --git a/spec/fixtures/api/schemas/pipeline_schedule.json b/spec/fixtures/api/schemas/pipeline_schedule.json index 16bbeacddbd..c76c6945117 100644 --- a/spec/fixtures/api/schemas/pipeline_schedule.json +++ b/spec/fixtures/api/schemas/pipeline_schedule.json @@ -34,14 +34,7 @@ }, "variables": { "type": ["array", "null"], - "items": { - "type": ["object", "null"], - "properties": { - "key": { "type": "string" }, - "value": { "type": "string" } - }, - "additionalProperties": false - } + "items": { "$ref": "pipeline_schedule_variable.json" } } }, "required": [ diff --git a/spec/fixtures/api/schemas/pipeline_schedule_variable.json b/spec/fixtures/api/schemas/pipeline_schedule_variable.json new file mode 100644 index 00000000000..f7ccb2d44a0 --- /dev/null +++ b/spec/fixtures/api/schemas/pipeline_schedule_variable.json @@ -0,0 +1,8 @@ +{ + "type": ["object", "null"], + "properties": { + "key": { "type": "string" }, + "value": { "type": "string" } + }, + "additionalProperties": false +} diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index b6a5a7ffbb5..e55e3617b5a 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -299,4 +299,160 @@ describe API::PipelineSchedules do end end end + + describe 'POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables' do + let(:params) { attributes_for(:ci_pipeline_schedule_variable) } + + let(:pipeline_schedule) do + create(:ci_pipeline_schedule, project: project, owner: developer) + end + + context 'authenticated user with valid permissions' do + context 'with required parameters' do + it 'creates pipeline_schedule_variable' do + expect do + post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables", developer), + params + end.to change { pipeline_schedule.variables.count }.by(1) + + expect(response).to have_http_status(:created) + expect(response).to match_response_schema('pipeline_schedule_variable') + expect(json_response['key']).to eq(params[:key]) + expect(json_response['value']).to eq(params[:value]) + end + end + + context 'without required parameters' do + it 'does not create pipeline_schedule_variable' do + post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables", developer) + + expect(response).to have_http_status(:bad_request) + end + end + + context 'when cron has validation error' do + it 'does not create pipeline_schedule_variable' do + post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables", developer), + params.merge('key' => '!?!?') + + expect(response).to have_http_status(:bad_request) + expect(json_response['message']).to have_key('key') + end + end + end + + context 'authenticated user with invalid permissions' do + it 'does not create pipeline_schedule_variable' do + post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables", user), params + + expect(response).to have_http_status(:not_found) + end + end + + context 'unauthenticated user' do + it 'does not create pipeline_schedule_variable' do + post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables"), params + + expect(response).to have_http_status(:unauthorized) + end + end + end + + describe 'PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + let(:pipeline_schedule) do + create(:ci_pipeline_schedule, project: project, owner: developer) + end + + let(:pipeline_schedule_variable) do + create(:ci_pipeline_schedule_variable, pipeline_schedule: pipeline_schedule) + end + + context 'authenticated user with valid permissions' do + it 'updates cron' do + put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", developer), + key: pipeline_schedule_variable.key, value: 'updated_value' + + expect(response).to have_http_status(:ok) + expect(response).to match_response_schema('pipeline_schedule_variable') + expect(json_response['key']).to eq('updated_value') + end + + context 'when cron has validation error' do + it 'does not update pipeline_schedule_variable' do + put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", developer), + key: '!?!?' + + expect(response).to have_http_status(:bad_request) + expect(json_response['message']).to have_key('key') + end + end + end + + context 'authenticated user with invalid permissions' do + it 'does not update pipeline_schedule_variable' do + put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", user) + + expect(response).to have_http_status(:not_found) + end + end + + context 'unauthenticated user' do + it 'does not update pipeline_schedule_variable' do + put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}") + + expect(response).to have_http_status(:unauthorized) + end + end + end + + describe 'DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do + let(:master) { create(:user) } + + let!(:pipeline_schedule) do + create(:ci_pipeline_schedule, project: project, owner: developer) + end + + let(:pipeline_schedule_variable) do + create(:ci_pipeline_schedule_variable, pipeline_schedule: pipeline_schedule) + end + + before do + project.add_master(master) + end + + context 'authenticated user with valid permissions' do + it 'deletes pipeline_schedule_variable' do + expect do + delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", master) + end.to change { project.pipeline_schedules.count }.by(-1) + + expect(response).to have_http_status(:accepted) + expect(response).to match_response_schema('pipeline_schedule_variable') + end + + it 'responds with 404 Not Found if requesting non-existing pipeline_schedule_variable' do + delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/____", master) + + expect(response).to have_http_status(:not_found) + end + end + + context 'authenticated user with invalid permissions' do # TODO: + let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: master) } + + it 'does not delete pipeline_schedule_variable' do + delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", developer) + + expect(response).to have_http_status(:forbidden) + end + end + + context 'unauthenticated user' do + it 'does not delete pipeline_schedule_variable' do + delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}") + + expect(response).to have_http_status(:unauthorized) + end + end + end end -- cgit v1.2.1 From fb8f32a92cdfe4cca24cb80a91e8fe48d6b0df25 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 22 Aug 2017 21:42:17 +0900 Subject: Finish spec --- lib/api/pipeline_schedules.rb | 2 +- spec/requests/api/pipeline_schedules_spec.rb | 22 ++++++---------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index ffb9a09834d..7a3f74006c6 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -184,7 +184,7 @@ module API not_found!('Variable') unless variable status :accepted - present variable, with: Entities::Variable + present variable.destroy, with: Entities::Variable end end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index e55e3617b5a..8239c26facd 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -330,7 +330,7 @@ describe API::PipelineSchedules do end end - context 'when cron has validation error' do + context 'when key has validation error' do it 'does not create pipeline_schedule_variable' do post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables", developer), params.merge('key' => '!?!?') @@ -368,23 +368,13 @@ describe API::PipelineSchedules do end context 'authenticated user with valid permissions' do - it 'updates cron' do + it 'updates pipeline_schedule_variable' do put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", developer), - key: pipeline_schedule_variable.key, value: 'updated_value' + value: 'updated_value' expect(response).to have_http_status(:ok) expect(response).to match_response_schema('pipeline_schedule_variable') - expect(json_response['key']).to eq('updated_value') - end - - context 'when cron has validation error' do - it 'does not update pipeline_schedule_variable' do - put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", developer), - key: '!?!?' - - expect(response).to have_http_status(:bad_request) - expect(json_response['message']).to have_key('key') - end + expect(json_response['value']).to eq('updated_value') end end @@ -412,7 +402,7 @@ describe API::PipelineSchedules do create(:ci_pipeline_schedule, project: project, owner: developer) end - let(:pipeline_schedule_variable) do + let!(:pipeline_schedule_variable) do create(:ci_pipeline_schedule_variable, pipeline_schedule: pipeline_schedule) end @@ -424,7 +414,7 @@ describe API::PipelineSchedules do it 'deletes pipeline_schedule_variable' do expect do delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/variables/#{pipeline_schedule_variable.key}", master) - end.to change { project.pipeline_schedules.count }.by(-1) + end.to change { Ci::PipelineScheduleVariable.count }.by(-1) expect(response).to have_http_status(:accepted) expect(response).to match_response_schema('pipeline_schedule_variable') -- cgit v1.2.1 From 362f2226a5febb7a3a82e86f4a83e87a870d67b3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 24 Aug 2017 21:51:46 +0900 Subject: Improve by zj nice catches --- doc/api/pipeline_schedules.md | 39 +++++++++++++++------------- lib/api/entities.rb | 2 +- lib/api/pipeline_schedules.rb | 37 ++++++++++---------------- spec/requests/api/pipeline_schedules_spec.rb | 8 +++--- 4 files changed, 39 insertions(+), 47 deletions(-) diff --git a/doc/api/pipeline_schedules.md b/doc/api/pipeline_schedules.md index bfa6fb731eb..c28f48e5fc6 100644 --- a/doc/api/pipeline_schedules.md +++ b/doc/api/pipeline_schedules.md @@ -290,12 +290,12 @@ Create a new variable of a pipeline schedule. POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables ``` -| Attribute | Type | required | Description | -|---------------|---------|----------|--------------------------| -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | -| `key` | string | yes | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed | -| `value` | string | yes | The `value` of a variable | +| Attribute | Type | required | Description | +|------------------------|----------------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed | +| `value` | string | yes | The `value` of a variable | ```sh curl --request POST --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "key=NEW_VARIABLE" --form "value=new value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables" @@ -316,12 +316,12 @@ Updates the variable of a pipeline schedule. PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key ``` -| Attribute | Type | required | Description | -|---------------|---------|----------|--------------------------| -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | -| `key` | string | yes | The `key` of a variable | -| `value` | string | yes | The `value` of a variable | +| Attribute | Type | required | Description | +|------------------------|----------------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable | +| `value` | string | yes | The `value` of a variable | ```sh curl --request PUT --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "value=updated value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE" @@ -342,18 +342,21 @@ Delete the variable of a pipeline schedule. DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key ``` -| Attribute | Type | required | Description | -|----------------|---------|----------|--------------------------| -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | -| `key` | string | yes | The `key` of a variable | +| Attribute | Type | required | Description | +|------------------------|----------------|----------|--------------------------| +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `pipeline_schedule_id` | integer | yes | The pipeline schedule id | +| `key` | string | yes | The `key` of a variable | ```sh curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE" ``` ```json -// Empty +{ + "key": "NEW_VARIABLE", + "value": "updated value" +} ``` [ce-34518]: https://gitlab.com/gitlab-org/gitlab-ce/issues/34518 \ No newline at end of file diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 81cf7039260..0092cc14e74 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -819,7 +819,7 @@ module API class Variable < Grape::Entity expose :key, :value - expose :protected?, as: :protected, if: -> (entity, options) { entity.respond_to?(:protected?) } + expose :protected?, as: :protected, if: -> (entity, _) { entity.respond_to?(:protected?) } end class Pipeline < PipelineBasic diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index 7a3f74006c6..a6414bfe3f4 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -33,8 +33,6 @@ module API get ':id/pipeline_schedules/:pipeline_schedule_id' do authorize! :read_pipeline_schedule, user_project - not_found!('PipelineSchedule') unless pipeline_schedule - present pipeline_schedule, with: Entities::PipelineScheduleDetails end @@ -75,8 +73,6 @@ module API end put ':id/pipeline_schedules/:pipeline_schedule_id' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :update_pipeline_schedule, pipeline_schedule if pipeline_schedule.update(declared_params(include_missing: false)) @@ -94,8 +90,6 @@ module API end post ':id/pipeline_schedules/:pipeline_schedule_id/take_ownership' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :update_pipeline_schedule, pipeline_schedule if pipeline_schedule.own!(current_user) @@ -113,8 +107,6 @@ module API end delete ':id/pipeline_schedules/:pipeline_schedule_id' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :admin_pipeline_schedule, pipeline_schedule destroy_conditionally!(pipeline_schedule) @@ -130,8 +122,6 @@ module API end post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :update_pipeline_schedule, pipeline_schedule variable_params = declared_params(include_missing: false) @@ -153,17 +143,12 @@ module API end put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :update_pipeline_schedule, pipeline_schedule - variable = pipeline_schedule.variables.find_by(key: params[:key]) - not_found!('Variable') unless variable - - if variable.update(declared_params(include_missing: false)) - present variable, with: Entities::Variable + if pipeline_schedule_variable.update(declared_params(include_missing: false)) + present pipeline_schedule_variable, with: Entities::Variable else - render_validation_error!(variable) + render_validation_error!(pipeline_schedule_variable) end end @@ -176,15 +161,10 @@ module API end delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do authorize! :read_pipeline_schedule, user_project - - not_found!('PipelineSchedule') unless pipeline_schedule authorize! :admin_pipeline_schedule, pipeline_schedule - variable = pipeline_schedule.variables.find_by(key: params[:key]) - not_found!('Variable') unless variable - status :accepted - present variable.destroy, with: Entities::Variable + present pipeline_schedule_variable.destroy, with: Entities::Variable end end @@ -194,6 +174,15 @@ module API user_project.pipeline_schedules .preload(:owner, :last_pipeline) .find_by(id: params.delete(:pipeline_schedule_id)) + + @pipeline_schedule || not_found!('Pipeline Schedule') + end + + def pipeline_schedule_variable + @pipeline_schedule_variable ||= + pipeline_schedule.variables.find_by(key: params[:key]) + + @pipeline_schedule_variable || not_found!('Pipeline Schedule Variable') end end end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 8239c26facd..86e834b5a22 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -303,7 +303,7 @@ describe API::PipelineSchedules do describe 'POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables' do let(:params) { attributes_for(:ci_pipeline_schedule_variable) } - let(:pipeline_schedule) do + set(:pipeline_schedule) do create(:ci_pipeline_schedule, project: project, owner: developer) end @@ -359,7 +359,7 @@ describe API::PipelineSchedules do end describe 'PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do - let(:pipeline_schedule) do + set(:pipeline_schedule) do create(:ci_pipeline_schedule, project: project, owner: developer) end @@ -398,7 +398,7 @@ describe API::PipelineSchedules do describe 'DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do let(:master) { create(:user) } - let!(:pipeline_schedule) do + set(:pipeline_schedule) do create(:ci_pipeline_schedule, project: project, owner: developer) end @@ -427,7 +427,7 @@ describe API::PipelineSchedules do end end - context 'authenticated user with invalid permissions' do # TODO: + context 'authenticated user with invalid permissions' do let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: master) } it 'does not delete pipeline_schedule_variable' do -- cgit v1.2.1 From bb22989c388bb7322e95af72c48d8422494d96e7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 30 Aug 2017 20:40:19 +0900 Subject: Improve def pipeline_schedule with authrozation code --- lib/api/pipeline_schedules.rb | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index a6414bfe3f4..51baf12e287 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -31,8 +31,6 @@ module API requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' end get ':id/pipeline_schedules/:pipeline_schedule_id' do - authorize! :read_pipeline_schedule, user_project - present pipeline_schedule, with: Entities::PipelineScheduleDetails end @@ -72,7 +70,6 @@ module API optional :active, type: Boolean, desc: 'The activation of pipeline schedule' end put ':id/pipeline_schedules/:pipeline_schedule_id' do - authorize! :read_pipeline_schedule, user_project authorize! :update_pipeline_schedule, pipeline_schedule if pipeline_schedule.update(declared_params(include_missing: false)) @@ -89,7 +86,6 @@ module API requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' end post ':id/pipeline_schedules/:pipeline_schedule_id/take_ownership' do - authorize! :read_pipeline_schedule, user_project authorize! :update_pipeline_schedule, pipeline_schedule if pipeline_schedule.own!(current_user) @@ -106,7 +102,6 @@ module API requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id' end delete ':id/pipeline_schedules/:pipeline_schedule_id' do - authorize! :read_pipeline_schedule, user_project authorize! :admin_pipeline_schedule, pipeline_schedule destroy_conditionally!(pipeline_schedule) @@ -121,7 +116,6 @@ module API requires :value, type: String, desc: 'The value of the variable' end post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do - authorize! :read_pipeline_schedule, user_project authorize! :update_pipeline_schedule, pipeline_schedule variable_params = declared_params(include_missing: false) @@ -142,7 +136,6 @@ module API optional :value, type: String, desc: 'The value of the variable' end put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do - authorize! :read_pipeline_schedule, user_project authorize! :update_pipeline_schedule, pipeline_schedule if pipeline_schedule_variable.update(declared_params(include_missing: false)) @@ -160,7 +153,6 @@ module API requires :key, type: String, desc: 'The key of the variable' end delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do - authorize! :read_pipeline_schedule, user_project authorize! :admin_pipeline_schedule, pipeline_schedule status :accepted @@ -171,18 +163,23 @@ module API helpers do def pipeline_schedule @pipeline_schedule ||= - user_project.pipeline_schedules - .preload(:owner, :last_pipeline) - .find_by(id: params.delete(:pipeline_schedule_id)) - - @pipeline_schedule || not_found!('Pipeline Schedule') + user_project + .pipeline_schedules + .preload(:owner, :last_pipeline) + .find_by(id: params.delete(:pipeline_schedule_id)).tap do |pipeline_schedule| + unless pipeline_schedule || can?(current_user, :read_pipeline_schedule, pipeline_schedule) + not_found!('Pipeline Schedule') + end + end end def pipeline_schedule_variable @pipeline_schedule_variable ||= - pipeline_schedule.variables.find_by(key: params[:key]) - - @pipeline_schedule_variable || not_found!('Pipeline Schedule Variable') + pipeline_schedule.variables.find_by(key: params[:key]).tap do |pipeline_schedule_variable| + unless pipeline_schedule_variable + not_found!('Pipeline Schedule Variable') + end + end end end end -- cgit v1.2.1 From 2f906430fa9efa61b7808e5849611fef6ecb59a5 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 4 Sep 2017 21:53:19 +0900 Subject: Fix security breaching --- lib/api/pipeline_schedules.rb | 2 +- spec/requests/api/pipeline_schedules_spec.rb | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index 51baf12e287..37f32411296 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -167,7 +167,7 @@ module API .pipeline_schedules .preload(:owner, :last_pipeline) .find_by(id: params.delete(:pipeline_schedule_id)).tap do |pipeline_schedule| - unless pipeline_schedule || can?(current_user, :read_pipeline_schedule, pipeline_schedule) + unless can?(current_user, :read_pipeline_schedule, pipeline_schedule) not_found!('Pipeline Schedule') end end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 86e834b5a22..f650df57383 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe API::PipelineSchedules do set(:developer) { create(:user) } set(:user) { create(:user) } - set(:project) { create(:project, :repository) } + set(:project) { create(:project, :repository, public_builds: false) } before do project.add_developer(developer) @@ -110,6 +110,18 @@ describe API::PipelineSchedules do end end + context 'authenticated user with insufficient permissions' do + before do + project.add_guest(user) + end + + it 'does not return pipeline_schedules list' do + get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user) + + expect(response).to have_http_status(:not_found) + end + end + context 'unauthenticated user' do it 'does not return pipeline_schedules list' do get api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}") -- cgit v1.2.1 From 75130a41ba19b80ac7b2300721915787ac4681bf Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 24 Aug 2017 17:08:32 +0900 Subject: Remove CreateTriggerRequestService and forbit to save variables on Ci::TriggerRequest --- app/models/ci/trigger_request.rb | 4 ++ app/services/ci/create_trigger_request_service.rb | 19 -------- lib/api/triggers.rb | 8 ++-- lib/api/v3/triggers.rb | 31 ++++++++----- spec/factories/ci/trigger_requests.rb | 17 +++---- spec/models/ci/trigger_request_spec.rb | 17 +++++++ .../ci/create_trigger_request_service_spec.rb | 52 ---------------------- 7 files changed, 53 insertions(+), 95 deletions(-) delete mode 100644 app/services/ci/create_trigger_request_service.rb create mode 100644 spec/models/ci/trigger_request_spec.rb delete mode 100644 spec/services/ci/create_trigger_request_service_spec.rb diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb index c58ce5c3717..4e274a57504 100644 --- a/app/models/ci/trigger_request.rb +++ b/app/models/ci/trigger_request.rb @@ -6,6 +6,10 @@ module Ci belongs_to :pipeline, foreign_key: :commit_id has_many :builds + # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # Ci::TriggerRequest doesn't save variables anymore. + validates :variables, absence: true + serialize :variables # rubocop:disable Cop/ActiveRecordSerialize def user_variables diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb deleted file mode 100644 index b2aa457bbd5..00000000000 --- a/app/services/ci/create_trigger_request_service.rb +++ /dev/null @@ -1,19 +0,0 @@ -# This class is deprecated because we're closing Ci::TriggerRequest. -# New class is PipelineTriggerService (app/services/ci/pipeline_trigger_service.rb) -# which is integrated with Ci::PipelineVariable instaed of Ci::TriggerRequest. -# We remove this class after we removed v1 and v3 API. This class is still being -# referred by such legacy code. -module Ci - module CreateTriggerRequestService - Result = Struct.new(:trigger_request, :pipeline) - - def self.execute(project, trigger, ref, variables = nil) - trigger_request = trigger.trigger_requests.create(variables: variables) - - pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref) - .execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request) - - Result.new(trigger_request, pipeline) - end - end -end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index dd6801664b1..276d3b5fb79 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -15,16 +15,16 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do + authenticate! + authorize! :admin_build, user_project + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } render_api_error!('variables needs to be a map of key-valued strings', 400) end - project = find_project(params[:id]) - not_found! unless project - - result = Ci::PipelineTriggerService.new(project, nil, params).execute + result = Ci::PipelineTriggerService.new(user_project, nil, params).execute not_found! unless result if result[:http_status] diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index e9d4c35307b..ffc1a38acbc 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -16,25 +16,32 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do - project = find_project(params[:id]) - trigger = Ci::Trigger.find_by_token(params[:token].to_s) - not_found! unless project && trigger - unauthorized! unless trigger.project == project + authenticate! + authorize! :admin_build, user_project # validate variables - variables = params[:variables].to_h - unless variables.all? { |key, value| key.is_a?(String) && value.is_a?(String) } + params[:variables] = params[:variables].to_h + unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } render_api_error!('variables needs to be a map of key-valued strings', 400) end - # create request and trigger builds - result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables) - pipeline = result.pipeline + result = Ci::PipelineTriggerService.new(user_project, nil, params).execute + not_found! unless result - if pipeline.persisted? - present result.trigger_request, with: ::API::V3::Entities::TriggerRequest + if result[:http_status] + render_api_error!(result[:message], result[:http_status]) else - render_validation_error!(pipeline) + pipeline = result[:pipeline] + trigger_request = pipeline.trigger_request + + # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # Ci::TriggerRequest doesn't save variables anymore. + # Although, to prevent braking compatibility, copying variables and present it as Ci::TriggerRequest. + pipeline.variables.each do |variable| + trigger_request.variables << { key: variable.key, value: variable.value } + end + + present trigger_request, with: ::API::V3::Entities::TriggerRequest end end diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index 10e0ab4fd3c..b6239385438 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -2,13 +2,14 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do trigger factory: :ci_trigger - factory :ci_trigger_request_with_variables do - variables do - { - TRIGGER_KEY_1: 'TRIGGER_VALUE_1', - TRIGGER_KEY_2: 'TRIGGER_VALUE_2' - } - end - end + # TODO: + # factory :ci_trigger_request_with_variables do + # variables do + # { + # TRIGGER_KEY_1: 'TRIGGER_VALUE_1', + # TRIGGER_KEY_2: 'TRIGGER_VALUE_2' + # } + # end + # end end end diff --git a/spec/models/ci/trigger_request_spec.rb b/spec/models/ci/trigger_request_spec.rb new file mode 100644 index 00000000000..32d6f835c73 --- /dev/null +++ b/spec/models/ci/trigger_request_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe Ci::TriggerRequest do + describe 'validation' do + it 'be invalid if saving a variable' do + trigger = build(:ci_trigger_request, variables: { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } ) + + expect(trigger.valid?).to be_falsey + end + + it 'be valid if not saving a variable' do + trigger = build(:ci_trigger_request) + + expect(trigger.valid?).to be_truthy + end + end +end diff --git a/spec/services/ci/create_trigger_request_service_spec.rb b/spec/services/ci/create_trigger_request_service_spec.rb deleted file mode 100644 index 8295813a1ca..00000000000 --- a/spec/services/ci/create_trigger_request_service_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require 'spec_helper' - -describe Ci::CreateTriggerRequestService do - let(:service) { described_class } - let(:project) { create(:project, :repository) } - let(:trigger) { create(:ci_trigger, project: project, owner: owner) } - let(:owner) { create(:user) } - - before do - stub_ci_pipeline_to_return_yaml_file - - project.add_developer(owner) - end - - describe '#execute' do - context 'valid params' do - subject { service.execute(project, trigger, 'master') } - - context 'without owner' do - it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } - it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } - it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } - it { expect(subject.pipeline).to be_trigger } - end - - context 'with owner' do - it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } - it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } - it { expect(subject.trigger_request.builds.first.user).to eq(owner) } - it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } - it { expect(subject.pipeline).to be_trigger } - it { expect(subject.pipeline.user).to eq(owner) } - end - end - - context 'no commit for ref' do - subject { service.execute(project, trigger, 'other-branch') } - - it { expect(subject.pipeline).not_to be_persisted } - end - - context 'no builds created' do - subject { service.execute(project, trigger, 'master') } - - before do - stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }') - end - - it { expect(subject.pipeline).not_to be_persisted } - end - end -end -- cgit v1.2.1 From acc7497855167d4f6ba481422112645a1a04a885 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 26 Aug 2017 04:04:57 +0900 Subject: Revert autheticate! in Trigger API --- lib/api/triggers.rb | 8 ++++---- lib/api/v3/triggers.rb | 8 ++++---- spec/factories/ci/builds.rb | 2 +- spec/factories/ci/trigger_requests.rb | 17 ++++++++--------- spec/features/projects/jobs_spec.rb | 2 +- spec/models/ci/build_spec.rb | 2 +- spec/requests/api/runner_spec.rb | 2 +- spec/requests/api/v3/triggers_spec.rb | 2 +- spec/views/projects/jobs/show.html.haml_spec.rb | 2 +- 9 files changed, 22 insertions(+), 23 deletions(-) diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 276d3b5fb79..dd6801664b1 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -15,16 +15,16 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do - authenticate! - authorize! :admin_build, user_project - # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } render_api_error!('variables needs to be a map of key-valued strings', 400) end - result = Ci::PipelineTriggerService.new(user_project, nil, params).execute + project = find_project(params[:id]) + not_found! unless project + + result = Ci::PipelineTriggerService.new(project, nil, params).execute not_found! unless result if result[:http_status] diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index ffc1a38acbc..e1da96104a5 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -16,16 +16,16 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do - authenticate! - authorize! :admin_build, user_project - # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } render_api_error!('variables needs to be a map of key-valued strings', 400) end - result = Ci::PipelineTriggerService.new(user_project, nil, params).execute + project = find_project(params[:id]) + not_found! unless project + + result = Ci::PipelineTriggerService.new(project, nil, params).execute not_found! unless result if result[:http_status] diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 25ec63de94a..592df038aa3 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -107,7 +107,7 @@ FactoryGirl.define do end trait :triggered do - trigger_request factory: :ci_trigger_request_with_variables + trigger_request factory: :ci_trigger_request_with_variables # TODO: end after(:build) do |build, evaluator| diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index b6239385438..10e0ab4fd3c 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -2,14 +2,13 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do trigger factory: :ci_trigger - # TODO: - # factory :ci_trigger_request_with_variables do - # variables do - # { - # TRIGGER_KEY_1: 'TRIGGER_VALUE_1', - # TRIGGER_KEY_2: 'TRIGGER_VALUE_2' - # } - # end - # end + factory :ci_trigger_request_with_variables do + variables do + { + TRIGGER_KEY_1: 'TRIGGER_VALUE_1', + TRIGGER_KEY_2: 'TRIGGER_VALUE_2' + } + end + end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 037ac00d39f..3f59a66403b 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -292,7 +292,7 @@ feature 'Jobs' do end feature 'Variables' do - let(:trigger_request) { create(:ci_trigger_request_with_variables) } + let(:trigger_request) { create(:ci_trigger_request_with_variables) } # TODO: let(:job) do create :ci_build, pipeline: pipeline, trigger_request: trigger_request diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 3fe3ec17d36..c0ac4816ae1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1492,7 +1492,7 @@ describe Ci::Build do context 'when build is for triggers' do let(:trigger) { create(:ci_trigger, project: project) } - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } + let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } # TODO: let(:user_trigger_variable) do { key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1', public: false } end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 993164aa8fe..d3b29426db1 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -559,7 +559,7 @@ describe API::Runner do before do trigger = create(:ci_trigger, project: project) - create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) + create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) # TODO: project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value') end diff --git a/spec/requests/api/v3/triggers_spec.rb b/spec/requests/api/v3/triggers_spec.rb index d4648136841..aca912dd427 100644 --- a/spec/requests/api/v3/triggers_spec.rb +++ b/spec/requests/api/v3/triggers_spec.rb @@ -37,7 +37,7 @@ describe API::V3::Triggers do it 'returns unauthorized if token is for different project' do post v3_api("/projects/#{project2.id}/trigger/builds"), options.merge(ref: 'master') - expect(response).to have_http_status(401) + expect(response).to have_http_status(404) end end diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb index 117f48450e2..7405df1742e 100644 --- a/spec/views/projects/jobs/show.html.haml_spec.rb +++ b/spec/views/projects/jobs/show.html.haml_spec.rb @@ -197,7 +197,7 @@ describe 'projects/jobs/show' do end describe 'shows trigger variables in sidebar' do - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } + let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } # TODO: before do build.trigger_request = trigger_request -- cgit v1.2.1 From cff104ec4b0dd2c53ed907ab7ca423b7c587dee8 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 28 Aug 2017 23:29:28 +0900 Subject: Fix spec --- app/models/ci/trigger_request.rb | 2 +- lib/api/v3/triggers.rb | 5 +++-- spec/factories/ci/builds.rb | 2 +- spec/factories/ci/trigger_requests.rb | 9 ++++----- spec/features/projects/jobs_spec.rb | 2 +- spec/models/ci/build_spec.rb | 2 +- spec/requests/api/runner_spec.rb | 2 +- spec/views/projects/jobs/show.html.haml_spec.rb | 2 +- 8 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb index 4e274a57504..2c860598281 100644 --- a/app/models/ci/trigger_request.rb +++ b/app/models/ci/trigger_request.rb @@ -6,7 +6,7 @@ module Ci belongs_to :pipeline, foreign_key: :commit_id has_many :builds - # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. # Ci::TriggerRequest doesn't save variables anymore. validates :variables, absence: true diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index e1da96104a5..497ccbe20c6 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -34,9 +34,10 @@ module API pipeline = result[:pipeline] trigger_request = pipeline.trigger_request - # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. # Ci::TriggerRequest doesn't save variables anymore. - # Although, to prevent braking compatibility, copying variables and present it as Ci::TriggerRequest. + # Here is copying Ci::PipelineVariable to Ci::TriggerRequest.variables for presenting the variables. + # The same endpoint in v4 API pressents Pipeline instead of TriggerRequest, so it doesn't need such a process. pipeline.variables.each do |variable| trigger_request.variables << { key: variable.key, value: variable.value } end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 592df038aa3..25ec63de94a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -107,7 +107,7 @@ FactoryGirl.define do end trait :triggered do - trigger_request factory: :ci_trigger_request_with_variables # TODO: + trigger_request factory: :ci_trigger_request_with_variables end after(:build) do |build, evaluator| diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index 10e0ab4fd3c..72dad8c418c 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -2,12 +2,11 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do trigger factory: :ci_trigger + # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # Ci::TriggerRequest doesn't save variables anymore, whereas old trigger requests still persist variables. factory :ci_trigger_request_with_variables do - variables do - { - TRIGGER_KEY_1: 'TRIGGER_VALUE_1', - TRIGGER_KEY_2: 'TRIGGER_VALUE_2' - } + after(:create) do |trigger_request, evaluator| + trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1', TRIGGER_KEY_2: 'TRIGGER_VALUE_2' } ) end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 3f59a66403b..037ac00d39f 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -292,7 +292,7 @@ feature 'Jobs' do end feature 'Variables' do - let(:trigger_request) { create(:ci_trigger_request_with_variables) } # TODO: + let(:trigger_request) { create(:ci_trigger_request_with_variables) } let(:job) do create :ci_build, pipeline: pipeline, trigger_request: trigger_request diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index c0ac4816ae1..3fe3ec17d36 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1492,7 +1492,7 @@ describe Ci::Build do context 'when build is for triggers' do let(:trigger) { create(:ci_trigger, project: project) } - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } # TODO: + let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } let(:user_trigger_variable) do { key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1', public: false } end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index d3b29426db1..993164aa8fe 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -559,7 +559,7 @@ describe API::Runner do before do trigger = create(:ci_trigger, project: project) - create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) # TODO: + create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value') end diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb index 7405df1742e..117f48450e2 100644 --- a/spec/views/projects/jobs/show.html.haml_spec.rb +++ b/spec/views/projects/jobs/show.html.haml_spec.rb @@ -197,7 +197,7 @@ describe 'projects/jobs/show' do end describe 'shows trigger variables in sidebar' do - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } # TODO: + let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } before do build.trigger_request = trigger_request -- cgit v1.2.1 From d614c431055286eaab3b82e810186ac19a2c4fd7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 00:17:56 +0900 Subject: Fix trigger_request.variables --- lib/api/v3/triggers.rb | 6 ++---- spec/models/ci/trigger_request_spec.rb | 4 ++-- spec/requests/api/v3/triggers_spec.rb | 3 ++- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index 497ccbe20c6..13cfb9fe38b 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -32,15 +32,13 @@ module API render_api_error!(result[:message], result[:http_status]) else pipeline = result[:pipeline] - trigger_request = pipeline.trigger_request + trigger_request = Ci::TriggerRequest.find_by(commit_id: pipeline.id) # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. # Ci::TriggerRequest doesn't save variables anymore. # Here is copying Ci::PipelineVariable to Ci::TriggerRequest.variables for presenting the variables. # The same endpoint in v4 API pressents Pipeline instead of TriggerRequest, so it doesn't need such a process. - pipeline.variables.each do |variable| - trigger_request.variables << { key: variable.key, value: variable.value } - end + trigger_request.variables = params[:variables] present trigger_request, with: ::API::V3::Entities::TriggerRequest end diff --git a/spec/models/ci/trigger_request_spec.rb b/spec/models/ci/trigger_request_spec.rb index 32d6f835c73..7dcf3528f73 100644 --- a/spec/models/ci/trigger_request_spec.rb +++ b/spec/models/ci/trigger_request_spec.rb @@ -5,13 +5,13 @@ describe Ci::TriggerRequest do it 'be invalid if saving a variable' do trigger = build(:ci_trigger_request, variables: { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } ) - expect(trigger.valid?).to be_falsey + expect(trigger).not_to be_valid end it 'be valid if not saving a variable' do trigger = build(:ci_trigger_request) - expect(trigger.valid?).to be_truthy + expect(trigger).to be_valid end end end diff --git a/spec/requests/api/v3/triggers_spec.rb b/spec/requests/api/v3/triggers_spec.rb index aca912dd427..7ccf387f2dc 100644 --- a/spec/requests/api/v3/triggers_spec.rb +++ b/spec/requests/api/v3/triggers_spec.rb @@ -80,7 +80,8 @@ describe API::V3::Triggers do post v3_api("/projects/#{project.id}/trigger/builds"), options.merge(variables: variables, ref: 'master') expect(response).to have_http_status(201) pipeline.builds.reload - expect(pipeline.builds.first.trigger_request.variables).to eq(variables) + expect(pipeline.variables.map { |v| { v.key => v.value } }.first).to eq(variables) + expect(json_response['variables']).to eq(variables) end end end -- cgit v1.2.1 From 3ae2038176b27673b06a040873fdbe19c94d67d3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 2 Sep 2017 02:37:11 +0900 Subject: Remove ci_trigger_request_with_variables --- app/presenters/ci/build_presenter.rb | 11 +++++++ app/views/projects/jobs/_sidebar.html.haml | 4 +-- spec/factories/ci/builds.rb | 2 +- spec/factories/ci/pipeline_variable_variables.rb | 8 ----- spec/factories/ci/pipeline_variables.rb | 8 +++++ spec/factories/ci/trigger_requests.rb | 8 ----- spec/features/projects/jobs_spec.rb | 40 +++++++++++++++++------- spec/models/ci/build_spec.rb | 28 ++++++++++++++--- spec/requests/api/runner_spec.rb | 31 ++++++++++++++---- spec/views/projects/jobs/show.html.haml_spec.rb | 16 ---------- 10 files changed, 100 insertions(+), 56 deletions(-) delete mode 100644 spec/factories/ci/pipeline_variable_variables.rb create mode 100644 spec/factories/ci/pipeline_variables.rb diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index c495c3f39bb..531ccd39cf2 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -17,5 +17,16 @@ module Ci "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" end end + + def old_or_new_trigger_variables + return @old_or_new_trigger_variables if defined?(@old_or_new_trigger_variables) + + if build.pipeline.variables.any? + @old_or_new_trigger_variables = + build.pipeline.variables&.map { |v| { v.key => v.value } } &.reduce({}, :merge) + else + @old_or_new_trigger_variables = build.trigger_request.variables + end + end end end diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml index f5d5bc7eda9..f22de357e5f 100644 --- a/app/views/projects/jobs/_sidebar.html.haml +++ b/app/views/projects/jobs/_sidebar.html.haml @@ -46,12 +46,12 @@ %span.build-light-text Token: #{@build.trigger_request.trigger.short_token} - - if @build.trigger_request.variables + - if @build.old_or_new_trigger_variables %p %button.btn.group.btn-group-justified.reveal-variables Reveal Variables %dl.js-build-variables.trigger-build-variables.hide - - @build.trigger_request.variables.each do |key, value| + - @build.old_or_new_trigger_variables.each do |key, value| %dt.js-build-variable.trigger-build-variable= key %dd.js-build-value.trigger-build-value= value diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 25ec63de94a..c2b59239af9 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -107,7 +107,7 @@ FactoryGirl.define do end trait :triggered do - trigger_request factory: :ci_trigger_request_with_variables + trigger_request factory: :ci_trigger_request end after(:build) do |build, evaluator| diff --git a/spec/factories/ci/pipeline_variable_variables.rb b/spec/factories/ci/pipeline_variable_variables.rb deleted file mode 100644 index 7c1a7faec08..00000000000 --- a/spec/factories/ci/pipeline_variable_variables.rb +++ /dev/null @@ -1,8 +0,0 @@ -FactoryGirl.define do - factory :ci_pipeline_variable, class: Ci::PipelineVariable do - sequence(:key) { |n| "VARIABLE_#{n}" } - value 'VARIABLE_VALUE' - - pipeline factory: :ci_empty_pipeline - end -end diff --git a/spec/factories/ci/pipeline_variables.rb b/spec/factories/ci/pipeline_variables.rb new file mode 100644 index 00000000000..7c1a7faec08 --- /dev/null +++ b/spec/factories/ci/pipeline_variables.rb @@ -0,0 +1,8 @@ +FactoryGirl.define do + factory :ci_pipeline_variable, class: Ci::PipelineVariable do + sequence(:key) { |n| "VARIABLE_#{n}" } + value 'VARIABLE_VALUE' + + pipeline factory: :ci_empty_pipeline + end +end diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index 72dad8c418c..40b8848920e 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -1,13 +1,5 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do trigger factory: :ci_trigger - - # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. - # Ci::TriggerRequest doesn't save variables anymore, whereas old trigger requests still persist variables. - factory :ci_trigger_request_with_variables do - after(:create) do |trigger_request, evaluator| - trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1', TRIGGER_KEY_2: 'TRIGGER_VALUE_2' } ) - end - end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 037ac00d39f..3b5c6966287 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -292,26 +292,44 @@ feature 'Jobs' do end feature 'Variables' do - let(:trigger_request) { create(:ci_trigger_request_with_variables) } + let(:trigger_request) { create(:ci_trigger_request) } let(:job) do create :ci_build, pipeline: pipeline, trigger_request: trigger_request end - before do - visit project_job_path(project, job) + shared_examples 'expected variables behavior' do + it 'shows variable key and value after click', js: true do + expect(page).to have_css('.reveal-variables') + expect(page).not_to have_css('.js-build-variable') + expect(page).not_to have_css('.js-build-value') + + click_button 'Reveal Variables' + + expect(page).not_to have_css('.reveal-variables') + expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') + expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + end end - it 'shows variable key and value after click', js: true do - expect(page).to have_css('.reveal-variables') - expect(page).not_to have_css('.js-build-variable') - expect(page).not_to have_css('.js-build-value') + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) - click_button 'Reveal Variables' + visit project_job_path(project, job) + end + + it_behaves_like 'expected variables behavior' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + + visit project_job_path(project, job) + end - expect(page).not_to have_css('.reveal-variables') - expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') - expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + it_behaves_like 'expected variables behavior' end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 3fe3ec17d36..08d22f166e4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1492,10 +1492,12 @@ describe Ci::Build do context 'when build is for triggers' do let(:trigger) { create(:ci_trigger, project: project) } - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } + let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } + let(:user_trigger_variable) do - { key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1', public: false } + { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false } end + let(:predefined_trigger_variable) do { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true } end @@ -1504,8 +1506,26 @@ describe Ci::Build do build.trigger_request = trigger_request end - it { is_expected.to include(user_trigger_variable) } - it { is_expected.to include(predefined_trigger_variable) } + shared_examples 'returns variables for triggers' do + it { is_expected.to include(user_trigger_variable) } + it { is_expected.to include(predefined_trigger_variable) } + end + + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + end + + it_behaves_like 'returns variables for triggers' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + end + + it_behaves_like 'returns variables for triggers' + end end context 'when pipeline has a variable' do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 993164aa8fe..6fede7c9f75 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -557,17 +557,36 @@ describe API::Runner do { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] end + let(:trigger) { create(:ci_trigger, project: project) } + let!(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, builds: [job], trigger: trigger) } + before do - trigger = create(:ci_trigger, project: project) - create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value') end - it 'returns variables for triggers' do - request_job + shared_examples 'expected variables behavior' do + it 'returns variables for triggers' do + request_job - expect(response).to have_http_status(201) - expect(json_response['variables']).to include(*expected_variables) + expect(response).to have_http_status(201) + expect(json_response['variables']).to include(*expected_variables) + end + end + + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } ) + end + + it_behaves_like 'expected variables behavior' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1') + end + + it_behaves_like 'expected variables behavior' end end diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb index 117f48450e2..d4279626e75 100644 --- a/spec/views/projects/jobs/show.html.haml_spec.rb +++ b/spec/views/projects/jobs/show.html.haml_spec.rb @@ -195,20 +195,4 @@ describe 'projects/jobs/show' do text: /\A\n#{Regexp.escape(commit_title)}\n\Z/) end end - - describe 'shows trigger variables in sidebar' do - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } - - before do - build.trigger_request = trigger_request - render - end - - it 'shows trigger variables in separate lines' do - expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_1') - expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_2') - expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_1') - expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_2') - end - end end -- cgit v1.2.1 From fa6b9acaf9759c58353f8407ff20a7d02b8edf92 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 4 Sep 2017 17:17:36 +0900 Subject: trigger_variables should consider trigger_request existstance always --- app/models/ci/build.rb | 14 +++++++++-- app/presenters/ci/build_presenter.rb | 11 --------- app/views/projects/jobs/_sidebar.html.haml | 8 +++---- spec/models/ci/build_spec.rb | 38 ++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 17 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ba3156154ac..e4f579b7897 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -221,14 +221,24 @@ module Ci variables += user_variables variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group variables += secret_variables(environment: environment) - variables += trigger_request.user_variables if trigger_request - variables += pipeline.variables.map(&:to_runner_variable) + variables += trigger_variables variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule variables += persisted_environment_variables if environment variables end + def trigger_variables + return [] unless trigger_request # or pipeline.trigger? + + @trigger_variables ||= + if pipeline.variables.any? # If it's swtiched to Ci::PipelineVariables + pipeline.variables.map(&:to_runner_variable) + else # else it's still using trigger_request.variables + trigger_request.user_variables # Deprecated + end + end + def merge_request return @merge_request if defined?(@merge_request) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 531ccd39cf2..c495c3f39bb 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -17,16 +17,5 @@ module Ci "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" end end - - def old_or_new_trigger_variables - return @old_or_new_trigger_variables if defined?(@old_or_new_trigger_variables) - - if build.pipeline.variables.any? - @old_or_new_trigger_variables = - build.pipeline.variables&.map { |v| { v.key => v.value } } &.reduce({}, :merge) - else - @old_or_new_trigger_variables = build.trigger_request.variables - end - end end end diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml index f22de357e5f..43e23bb2200 100644 --- a/app/views/projects/jobs/_sidebar.html.haml +++ b/app/views/projects/jobs/_sidebar.html.haml @@ -46,14 +46,14 @@ %span.build-light-text Token: #{@build.trigger_request.trigger.short_token} - - if @build.old_or_new_trigger_variables + - if @build.trigger_variables.any? %p %button.btn.group.btn-group-justified.reveal-variables Reveal Variables %dl.js-build-variables.trigger-build-variables.hide - - @build.old_or_new_trigger_variables.each do |key, value| - %dt.js-build-variable.trigger-build-variable= key - %dd.js-build-value.trigger-build-value= value + - @build.trigger_variables.each do |trigger_variable| + %dt.js-build-variable.trigger-build-variable= trigger_variable[:key] + %dd.js-build-value.trigger-build-value= trigger_variable[:value] %div{ class: (@build.pipeline.stages_count > 1 ? "block" : "block-last") } %p diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 08d22f166e4..137c460cce8 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1690,6 +1690,44 @@ describe Ci::Build do end end + describe '#trigger_variables' do + let(:build) { create(:ci_build, pipeline: pipeline, trigger_request: trigger_request) } + let(:trigger) { create(:ci_trigger, project: project) } + let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } + + subject { build.trigger_variables } + + it { is_expected.to eq(true) } + + context 'when variable is stored in ci_pipeline_variables' do + let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } + + context 'when pipeline is triggered by trigger API' do + it 'returns variables' do + is_expected.to eq([pipeline_variable.to_runner_variable]) + end + end + + context 'when pipeline is not triggered by trigger API' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'does not return variables' do + is_expected.to eq([]) + end + end + end + + context 'when variable is stored in ci_trigger_requests.variables' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + end + + it 'returns variables' do + is_expected.to eq(trigger_request.user_variables) + end + end + end + describe 'state transition: any => [:pending]' do let(:build) { create(:ci_build, :created) } -- cgit v1.2.1 From b354754f234055ac8bbb4b02cc9524d6da3843c3 Mon Sep 17 00:00:00 2001 From: winh Date: Mon, 4 Sep 2017 15:16:01 +0200 Subject: Avoid @extend for .text-danger in dropdowns --- app/assets/stylesheets/framework/dropdowns.scss | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index a49f9f99872..e29e1806b38 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -756,7 +756,7 @@ // make sure the text color is not overriden &.text-danger { - @extend .text-danger; + color: $brand-danger; } &.is-focused, @@ -765,6 +765,11 @@ &:focus { background-color: $dropdown-item-hover-bg; color: $gl-text-color; + + // make sure the text color is not overriden + &.text-danger { + color: $brand-danger; + } } &.is-active { -- cgit v1.2.1 From 5b88bd81f52b454c80c54bef9951bdb8cf607238 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 4 Sep 2017 22:21:47 +0900 Subject: Move trigger_variables to presenter --- app/models/ci/build.rb | 14 ++--------- app/presenters/ci/build_presenter.rb | 11 +++++++++ lib/gitlab/gitaly_client.rb | 3 ++- spec/models/ci/build_spec.rb | 38 ------------------------------ spec/presenters/ci/build_presenter_spec.rb | 34 ++++++++++++++++++++++++++ 5 files changed, 49 insertions(+), 51 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index e4f579b7897..ba3156154ac 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -221,24 +221,14 @@ module Ci variables += user_variables variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group variables += secret_variables(environment: environment) - variables += trigger_variables + variables += trigger_request.user_variables if trigger_request + variables += pipeline.variables.map(&:to_runner_variable) variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule variables += persisted_environment_variables if environment variables end - def trigger_variables - return [] unless trigger_request # or pipeline.trigger? - - @trigger_variables ||= - if pipeline.variables.any? # If it's swtiched to Ci::PipelineVariables - pipeline.variables.map(&:to_runner_variable) - else # else it's still using trigger_request.variables - trigger_request.user_variables # Deprecated - end - end - def merge_request return @merge_request if defined?(@merge_request) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index c495c3f39bb..255475e1fe6 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -17,5 +17,16 @@ module Ci "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" end end + + def trigger_variables + return [] unless trigger_request + + @trigger_variables ||= + if pipeline.variables.any? + pipeline.variables.map(&:to_runner_variable) + else + trigger_request.user_variables + end + end end end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 9a5f4f598b2..72065811528 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -23,7 +23,8 @@ module Gitlab klass = Gitaly.const_get(name.to_s.camelcase.to_sym).const_get(:Stub) addr = address(storage) addr = addr.sub(%r{^tcp://}, '') if URI(addr).scheme == 'tcp' - klass.new(addr, :this_channel_is_insecure) + timeout = 10 if Rails.env.test? + klass.new(addr, :this_channel_is_insecure, timeout: timeout) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 137c460cce8..08d22f166e4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1690,44 +1690,6 @@ describe Ci::Build do end end - describe '#trigger_variables' do - let(:build) { create(:ci_build, pipeline: pipeline, trigger_request: trigger_request) } - let(:trigger) { create(:ci_trigger, project: project) } - let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } - - subject { build.trigger_variables } - - it { is_expected.to eq(true) } - - context 'when variable is stored in ci_pipeline_variables' do - let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } - - context 'when pipeline is triggered by trigger API' do - it 'returns variables' do - is_expected.to eq([pipeline_variable.to_runner_variable]) - end - end - - context 'when pipeline is not triggered by trigger API' do - let(:build) { create(:ci_build, pipeline: pipeline) } - - it 'does not return variables' do - is_expected.to eq([]) - end - end - end - - context 'when variable is stored in ci_trigger_requests.variables' do - before do - trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) - end - - it 'returns variables' do - is_expected.to eq(trigger_request.user_variables) - end - end - end - describe 'state transition: any => [:pending]' do let(:build) { create(:ci_build, :created) } diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index a7a34ecac72..1a8001be6ab 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -100,4 +100,38 @@ describe Ci::BuildPresenter do end end end + + describe '#trigger_variables' do + let(:build) { create(:ci_build, pipeline: pipeline, trigger_request: trigger_request) } + let(:trigger) { create(:ci_trigger, project: project) } + let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } + + context 'when variable is stored in ci_pipeline_variables' do + let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } + + context 'when pipeline is triggered by trigger API' do + it 'returns variables' do + expect(presenter.trigger_variables).to eq([pipeline_variable.to_runner_variable]) + end + end + + context 'when pipeline is not triggered by trigger API' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'does not return variables' do + expect(presenter.trigger_variables).to eq([]) + end + end + end + + context 'when variable is stored in ci_trigger_requests.variables' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + end + + it 'returns variables' do + expect(presenter.trigger_variables).to eq(trigger_request.user_variables) + end + end + end end -- cgit v1.2.1 From 209549c95fb0d4ba3196cc8ed888f7c024216ebb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 4 Sep 2017 22:38:18 +0900 Subject: Revert iunnecessary change(gitaly) --- lib/gitlab/gitaly_client.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 72065811528..9a5f4f598b2 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -23,8 +23,7 @@ module Gitlab klass = Gitaly.const_get(name.to_s.camelcase.to_sym).const_get(:Stub) addr = address(storage) addr = addr.sub(%r{^tcp://}, '') if URI(addr).scheme == 'tcp' - timeout = 10 if Rails.env.test? - klass.new(addr, :this_channel_is_insecure, timeout: timeout) + klass.new(addr, :this_channel_is_insecure) end end end -- cgit v1.2.1 From 2065a2d1d9034169d42b06d331fe06b4772b6df4 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 4 Sep 2017 18:07:37 +0200 Subject: attempt to fix random import file spec error --- spec/features/projects/import_export/import_file_spec.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 2eb6fab129d..6d1caed61fd 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -18,23 +18,25 @@ feature 'Import/Export - project import integration test', js: true do context 'when selecting the namespace' do let(:user) { create(:admin) } - let!(:namespace) { create(:namespace, name: "asd", owner: user) } + let!(:namespace) { create(:namespace, name: "asd" + SecureRandom.hex, path: SecureRandom.hex, owner: user) } + let(:project_path) { 'test-project-path' + SecureRandom.hex } context 'prefilled the path' do scenario 'user imports an exported project successfully' do visit new_project_path select2(namespace.id, from: '#project_namespace_id') - fill_in :project_path, with: 'test-project-path', visible: true + fill_in :project_path, with: project_path, visible: true click_link 'GitLab export' expect(page).to have_content('Import an exported GitLab project') - expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=test-project-path") - expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path\z/).and_call_original + expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") + #expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path#{project_path}\z/).and_call_original attach_file('file', file) + click_on 'Import project' - expect { click_on 'Import project' }.to change { Project.count }.by(1) + expect(Project.count).to eq(1) project = Project.last expect(project).not_to be_nil @@ -64,7 +66,7 @@ feature 'Import/Export - project import integration test', js: true do end scenario 'invalid project' do - namespace = create(:namespace, name: "asd", owner: user) + namespace = create(:namespace, name: "asd" + SecureRandom.hex, owner: user) project = create(:project, namespace: namespace) visit new_project_path -- cgit v1.2.1 From af374be863de648d89f1571f5c311ec9b761294d Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 4 Sep 2017 18:17:37 +0200 Subject: refactor spec --- spec/features/projects/import_export/import_file_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 6d1caed61fd..bc6b6989a3b 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -18,7 +18,7 @@ feature 'Import/Export - project import integration test', js: true do context 'when selecting the namespace' do let(:user) { create(:admin) } - let!(:namespace) { create(:namespace, name: "asd" + SecureRandom.hex, path: SecureRandom.hex, owner: user) } + let!(:namespace) { create(:namespace, name: 'asd', owner: user) } let(:project_path) { 'test-project-path' + SecureRandom.hex } context 'prefilled the path' do @@ -31,7 +31,7 @@ feature 'Import/Export - project import integration test', js: true do expect(page).to have_content('Import an exported GitLab project') expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") - #expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path#{project_path}\z/).and_call_original + expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path.*\z/).and_call_original attach_file('file', file) click_on 'Import project' @@ -66,7 +66,7 @@ feature 'Import/Export - project import integration test', js: true do end scenario 'invalid project' do - namespace = create(:namespace, name: "asd" + SecureRandom.hex, owner: user) + namespace = create(:namespace, name: 'asdf', owner: user) project = create(:project, namespace: namespace) visit new_project_path -- cgit v1.2.1 From 48f017d1e84498eec38d276d94918021a985bfee Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 5 Sep 2017 01:22:57 +0900 Subject: Use pipeline.trigger_requests.last --- lib/api/v3/triggers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index 13cfb9fe38b..534911fde5c 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -32,12 +32,12 @@ module API render_api_error!(result[:message], result[:http_status]) else pipeline = result[:pipeline] - trigger_request = Ci::TriggerRequest.find_by(commit_id: pipeline.id) # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. # Ci::TriggerRequest doesn't save variables anymore. # Here is copying Ci::PipelineVariable to Ci::TriggerRequest.variables for presenting the variables. # The same endpoint in v4 API pressents Pipeline instead of TriggerRequest, so it doesn't need such a process. + trigger_request = pipeline.trigger_requests.last trigger_request.variables = params[:variables] present trigger_request, with: ::API::V3::Entities::TriggerRequest -- cgit v1.2.1 From 48115be509ce00120d0609f5f18a5bc3804bb21f Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 30 Aug 2017 12:00:39 +0100 Subject: Add a system check for the git user's custom SSH configuration --- .../37204-deprecate-git-user-manual-ssh-config.yml | 5 ++ doc/ssh/README.md | 32 +++++++++ .../app/git_user_default_ssh_config_check.rb | 69 +++++++++++++++++++ lib/tasks/gitlab/check.rake | 1 + .../app/git_user_default_ssh_config_check_spec.rb | 79 ++++++++++++++++++++++ 5 files changed, 186 insertions(+) create mode 100644 changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml create mode 100644 lib/system_check/app/git_user_default_ssh_config_check.rb create mode 100644 spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb diff --git a/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml b/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml new file mode 100644 index 00000000000..593e74593c4 --- /dev/null +++ b/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml @@ -0,0 +1,5 @@ +--- +title: Deprecate custom SSH client configuration for the git user +merge_request: 13930 +author: +type: deprecated diff --git a/doc/ssh/README.md b/doc/ssh/README.md index cf28f1a2eca..793de9d777c 100644 --- a/doc/ssh/README.md +++ b/doc/ssh/README.md @@ -193,6 +193,38 @@ How to add your SSH key to Eclipse: https://wiki.eclipse.org/EGit/User_Guide#Ecl [winputty]: https://the.earth.li/~sgtatham/putty/0.67/htmldoc/Chapter8.html#pubkey-puttygen +## SSH on the GitLab server + +GitLab integrates with the system-installed SSH daemon, designating a user +(typically named `git`) through which all access requests are handled. Users +connecting to the GitLab server over SSH are identified by their SSH key instead +of their username. + +SSH *client* operations performed on the GitLab server wil be executed as this +user. Although it is possible to modify the SSH configuration for this user to, +e.g., provide a private SSH key to authenticate these requests by, this practice +is **not supported** and is strongly discouraged as it presents significant +security risks. + +The GitLab check process includes a check for this condition, and will direct you +to this section if your server is configured like this, e.g.: + +``` +$ gitlab-rake gitlab:check +# ... +Git user has default SSH configuration? ... no + Try fixing it: + mkdir ~/gitlab-check-backup-1504540051 + sudo mv /var/lib/git/.ssh/id_rsa ~/gitlab-check-backup-1504540051 + sudo mv /var/lib/git/.ssh/id_rsa.pub ~/gitlab-check-backup-1504540051 + For more information see: + doc/ssh/README.md in section "SSH on the GitLab server" + Please fix the error above and rerun the checks. +``` + +Remove the custom configuration as soon as you're able to. These customizations +are *explicitly not supported* and may stop working at any time. + ## Troubleshooting If on Git clone you are prompted for a password like `git@gitlab.com's password:` diff --git a/lib/system_check/app/git_user_default_ssh_config_check.rb b/lib/system_check/app/git_user_default_ssh_config_check.rb new file mode 100644 index 00000000000..7b486d78cf0 --- /dev/null +++ b/lib/system_check/app/git_user_default_ssh_config_check.rb @@ -0,0 +1,69 @@ +module SystemCheck + module App + class GitUserDefaultSSHConfigCheck < SystemCheck::BaseCheck + # These files are allowed in the .ssh directory. The `config` file is not + # whitelisted as it may change the SSH client's behaviour dramatically. + WHITELIST = %w[ + authorized_keys + authorized_keys2 + known_hosts + ].freeze + + set_name 'Git user has default SSH configuration?' + set_skip_reason 'skipped (git user is not present or configured)' + + def skip? + !home_dir || !File.directory?(home_dir) + end + + def check? + forbidden_files.empty? + end + + def show_error + backup_dir = "~/gitlab-check-backup-#{Time.now.to_i}" + + instructions = forbidden_files.map do |filename| + "sudo mv #{Shellwords.escape(filename)} #{backup_dir}" + end + + try_fixing_it("mkdir #{backup_dir}", *instructions) + for_more_information('doc/ssh/README.md in section "SSH on the GitLab server"') + fix_and_rerun + end + + private + + def git_user + Gitlab.config.gitlab.user + end + + def home_dir + return @home_dir if defined?(@home_dir) + + @home_dir = + begin + File.expand_path("~#{git_user}") + rescue ArgumentError + nil + end + end + + def ssh_dir + return nil unless home_dir + + File.join(home_dir, '.ssh') + end + + def forbidden_files + @forbidden_files ||= + begin + present = Dir[File.join(ssh_dir, '*')] + whitelisted = WHITELIST.map { |basename| File.join(ssh_dir, basename) } + + present - whitelisted + end + end + end + end +end diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake index 1bd36bbe20a..92a3f503fcb 100644 --- a/lib/tasks/gitlab/check.rake +++ b/lib/tasks/gitlab/check.rake @@ -33,6 +33,7 @@ namespace :gitlab do SystemCheck::App::RedisVersionCheck, SystemCheck::App::RubyVersionCheck, SystemCheck::App::GitVersionCheck, + SystemCheck::App::GitUserDefaultSSHConfigCheck, SystemCheck::App::ActiveUsersCheck ] diff --git a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb new file mode 100644 index 00000000000..7125bfcab59 --- /dev/null +++ b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb @@ -0,0 +1,79 @@ +require 'spec_helper' + +describe SystemCheck::App::GitUserDefaultSSHConfigCheck do + let(:username) { '_this_user_will_not_exist_unless_it_is_stubbed' } + let(:base_dir) { Dir.mktmpdir } + let(:home_dir) { File.join(base_dir, "/var/lib/#{username}") } + let(:ssh_dir) { File.join(home_dir, '.ssh') } + let(:forbidden_file) { 'id_rsa' } + + before do + allow(Gitlab.config.gitlab).to receive(:user).and_return(username) + end + + after do + FileUtils.rm_rf(base_dir) + end + + it 'only whitelists safe files' do + expect(described_class::WHITELIST).to contain_exactly('authorized_keys', 'authorized_keys2', 'known_hosts') + end + + describe '#skip?' do + subject { described_class.new.skip? } + + where(user_exists: [true, false], home_dir_exists: [true, false]) + + with_them do + let(:expected_result) { !user_exists || !home_dir_exists } + + before do + stub_user if user_exists + stub_home_dir if home_dir_exists + end + + it { is_expected.to eq(expected_result) } + end + end + + describe '#check?' do + subject { described_class.new.check? } + + before do + stub_user + end + + it 'fails if a forbidden file exists' do + stub_ssh_file(forbidden_file) + + is_expected.to be_falsy + end + + it "succeeds if the SSH directory doesn't exist" do + FileUtils.rm_rf(ssh_dir) + + is_expected.to be_truthy + end + + it 'succeeds if all the whitelisted files exist' do + described_class::WHITELIST.each do |filename| + stub_ssh_file(filename) + end + + is_expected.to be_truthy + end + end + + def stub_user + allow(File).to receive(:expand_path).with("~#{username}").and_return(home_dir) + end + + def stub_home_dir + FileUtils.mkdir_p(home_dir) + end + + def stub_ssh_file(filename) + FileUtils.mkdir_p(ssh_dir) + FileUtils.touch(File.join(ssh_dir, filename)) + end +end -- cgit v1.2.1 From 7831e5cabacad8af5a0a7fcba0dd3a4153c723fa Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 4 Sep 2017 20:03:59 +0200 Subject: update regex --- spec/features/projects/import_export/import_file_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index bc6b6989a3b..ad2db1a34f4 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -31,7 +31,7 @@ feature 'Import/Export - project import integration test', js: true do expect(page).to have_content('Import an exported GitLab project') expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") - expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path.*\z/).and_call_original + expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path\h*\z/).and_call_original attach_file('file', file) click_on 'Import project' -- cgit v1.2.1 From 27cc09c2b49e9cc8d0680e6370900ca75f018c16 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Mon, 4 Sep 2017 21:38:58 +0200 Subject: Fix flaky-examples-test job to not run for docs --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index aa7659d1b41..b08c441028b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -212,7 +212,6 @@ update-tests-metadata: flaky-examples-check: <<: *dedicated-runner - <<: *except-docs image: ruby:2.3-alpine services: [] before_script: [] @@ -227,6 +226,7 @@ flaky-examples-check: - branches except: - master + - /(^docs[\/-].*|.*-docs$)/ artifacts: expire_in: 30d paths: -- cgit v1.2.1 From 3c1dfcc0323bac1a932b1a7b3fd5df63c259d5cc Mon Sep 17 00:00:00 2001 From: winh Date: Mon, 4 Sep 2017 22:10:59 +0200 Subject: Make dropdowns on new issue page consistent --- app/assets/stylesheets/framework/dropdowns.scss | 2 ++ app/assets/stylesheets/pages/issues.scss | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index a45d5a6dca0..377cd3315e5 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -728,6 +728,8 @@ @mixin new-style-dropdown($selector: '') { #{$selector}.dropdown-menu, #{$selector}.dropdown-menu-nav { + margin-bottom: 24px; + li { padding: 0 1px; diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index e2177f96aee..fe748ff7f0e 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -143,8 +143,12 @@ ul.related-merge-requests > li { } } -.issue-form .select2-container { - width: 250px !important; +.issue-form { + @include new-style-dropdown; + + .select2-container { + width: 250px !important; + } } .issues-footer { -- cgit v1.2.1 From b282e174dc802dcfe269a825fa3a873137f7fc1e Mon Sep 17 00:00:00 2001 From: winh Date: Tue, 15 Aug 2017 14:07:56 +0200 Subject: Make target branch dropdown on merge request edit page consistent --- app/assets/stylesheets/framework/selects.scss | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/assets/stylesheets/framework/selects.scss b/app/assets/stylesheets/framework/selects.scss index a39927eb0df..3bdf2bd11be 100644 --- a/app/assets/stylesheets/framework/selects.scss +++ b/app/assets/stylesheets/framework/selects.scss @@ -268,13 +268,21 @@ // TODO: change global style .ajax-project-dropdown, body[data-page="projects:new"] #select2-drop, +body[data-page="projects:merge_requests:edit"] #select2-drop, body[data-page="projects:blob:new"] #select2-drop, body[data-page="profiles:show"] #select2-drop, body[data-page="projects:blob:edit"] #select2-drop { &.select2-drop { + border: 1px solid $dropdown-border-color; + border-radius: $border-radius-base; color: $gl-text-color; } + &.select2-drop-above { + border-top: none; + margin-top: -4px; + } + .select2-results { .select2-no-results, .select2-searching, -- cgit v1.2.1 From cf9c54bd312dcb7b4b7f0602e83013d8d32a9413 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sun, 3 Sep 2017 16:34:50 +0900 Subject: Add my_reaction_emoji param to /issues API --- app/finders/issues_finder.rb | 1 + changelogs/unreleased/issue-api-my-reaction.yml | 5 ++ doc/api/issues.md | 94 +++++++++++++------------ lib/api/issues.rb | 1 + spec/requests/api/issues_spec.rb | 10 +++ 5 files changed, 67 insertions(+), 44 deletions(-) create mode 100644 changelogs/unreleased/issue-api-my-reaction.yml diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index aa9cef6b08c..d2275139c42 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -14,6 +14,7 @@ # search: string # label_name: string # sort: string +# my_reaction_emoji: string # class IssuesFinder < IssuableFinder CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER diff --git a/changelogs/unreleased/issue-api-my-reaction.yml b/changelogs/unreleased/issue-api-my-reaction.yml new file mode 100644 index 00000000000..8b772ef13e8 --- /dev/null +++ b/changelogs/unreleased/issue-api-my-reaction.yml @@ -0,0 +1,5 @@ +--- +title: Add my_reaction_emoji param to /issues API +merge_request: 14016 +author: Hiroyuki Sato +type: added diff --git a/doc/api/issues.md b/doc/api/issues.md index 765246142c1..f8f9db484e2 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -30,20 +30,22 @@ GET /issues?milestone=1.0.0&state=opened GET /issues?iids[]=42&iids[]=43 GET /issues?author_id=5 GET /issues?assignee_id=5 -``` - -| Attribute | Type | Required | Description | -|-------------|----------------|----------|-----------------------------------------------------------------------------------------------------------------------------| -| `state` | string | no | Return all issues or just those that are `opened` or `closed` | -| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | -| `milestone` | string | no | The milestone title | -| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `author_id` | integer | no | Return issues created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me`. _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | -| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | -| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | -| `search` | string | no | Search issues against their `title` and `description` | +GET /issues?my_reaction_emoji=star +``` + +| Attribute | Type | Required | Description | +| ------------------- | ---------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | +| `state` | string | no | Return all issues or just those that are `opened` or `closed` | +| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | +| `milestone` | string | no | The milestone title | +| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `author_id` | integer | no | Return issues created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me`. _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | +| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | +| `search` | string | no | Search issues against their `title` and `description` | ```bash curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/issues @@ -131,21 +133,23 @@ GET /groups/:id/issues?iids[]=42&iids[]=43 GET /groups/:id/issues?search=issue+title+or+description GET /groups/:id/issues?author_id=5 GET /groups/:id/issues?assignee_id=5 +GET /groups/:id/issues?my_reaction_emoji=star ``` -| Attribute | Type | Required | Description | -|-------------|----------------|----------|-----------------------------------------------------------------------------------------------------------------------------| -| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | -| `state` | string | no | Return all issues or just those that are `opened` or `closed` | -| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | -| `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | -| `milestone` | string | no | The milestone title | -| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | -| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | -| `search` | string | no | Search group issues against their `title` and `description` | +| Attribute | Type | Required | Description | +| ------------------- | ---------------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | +| `state` | string | no | Return all issues or just those that are `opened` or `closed` | +| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | +| `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | +| `milestone` | string | no | The milestone title | +| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | +| `search` | string | no | Search group issues against their `title` and `description` | ```bash @@ -234,23 +238,25 @@ GET /projects/:id/issues?iids[]=42&iids[]=43 GET /projects/:id/issues?search=issue+title+or+description GET /projects/:id/issues?author_id=5 GET /projects/:id/issues?assignee_id=5 -``` - -| Attribute | Type | Required | Description | -|-------------|----------------|----------|-----------------------------------------------------------------------------------------------------------------------------| -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `iids[]` | Array[integer] | no | Return only the milestone having the given `iid` | -| `state` | string | no | Return all issues or just those that are `opened` or `closed` | -| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | -| `milestone` | string | no | The milestone title | -| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | -| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | -| `search` | string | no | Search project issues against their `title` and `description` | -| `created_after` | datetime | no | Return issues created after the given time (inclusive) | -| `created_before` | datetime | no | Return issues created before the given time (inclusive) | +GET /projects/:id/issues?my_reaction_emoji=star +``` + +| Attribute | Type | Required | Description | +| ------------------- | ---------------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `iids[]` | Array[integer] | no | Return only the milestone having the given `iid` | +| `state` | string | no | Return all issues or just those that are `opened` or `closed` | +| `labels` | string | no | Comma-separated list of label names, issues must have all labels to be returned. `No+Label` lists all issues with no labels | +| `milestone` | string | no | The milestone title | +| `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | +| `search` | string | no | Search project issues against their `title` and `description` | +| `created_after` | datetime | no | Return issues created after the given time (inclusive) | +| `created_before` | datetime | no | Return issues created before the given time (inclusive) | ```bash curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues diff --git a/lib/api/issues.rb b/lib/api/issues.rb index e4c2c390853..1729df2aad0 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -36,6 +36,7 @@ module API optional :assignee_id, type: Integer, desc: 'Return issues which are assigned to the user with the given ID' optional :scope, type: String, values: %w[created-by-me assigned-to-me all], desc: 'Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`' + optional :my_reaction_emoji, type: String, desc: 'Return issues reacted by the authenticated user by the given emoji' use :pagination end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index dee75c96b86..1583d1c2435 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -138,6 +138,16 @@ describe API::Issues, :mailer do expect(first_issue['id']).to eq(issue2.id) end + it 'returns issues reacted by the authenticated user by the given emoji' do + issue2 = create(:issue, project: project, author: user, assignees: [user]) + award_emoji = create(:award_emoji, awardable: issue2, user: user2, name: 'star') + + get api('/issues', user2), my_reaction_emoji: award_emoji.name, scope: 'all' + + expect_paginated_array_response(size: 1) + expect(first_issue['id']).to eq(issue2.id) + end + it 'returns issues matching given search string for title' do get api("/issues", user), search: issue.title -- cgit v1.2.1 From 7e42711659d514be77119698d2611830fb83090d Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 4 Sep 2017 10:43:14 +0900 Subject: Add my_reaction_emoji param to /merge_requests API --- app/finders/merge_requests_finder.rb | 1 + changelogs/unreleased/issue-api-my-reaction.yml | 2 +- doc/api/merge_requests.md | 30 ++++++++++++++----------- lib/api/merge_requests.rb | 1 + spec/requests/api/merge_requests_spec.rb | 12 ++++++++++ 5 files changed, 32 insertions(+), 14 deletions(-) diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 771da3d441d..d0687d28c21 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -16,6 +16,7 @@ # label_name: string # sort: string # non_archived: boolean +# my_reaction_emoji: string # class MergeRequestsFinder < IssuableFinder def klass diff --git a/changelogs/unreleased/issue-api-my-reaction.yml b/changelogs/unreleased/issue-api-my-reaction.yml index 8b772ef13e8..1c12478fbc0 100644 --- a/changelogs/unreleased/issue-api-my-reaction.yml +++ b/changelogs/unreleased/issue-api-my-reaction.yml @@ -1,5 +1,5 @@ --- -title: Add my_reaction_emoji param to /issues API +title: Add my_reaction_emoji param to /issues and /merge_requests API merge_request: 14016 author: Hiroyuki Sato type: added diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 4f67aa4b9d4..a0b31d3633c 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -22,24 +22,26 @@ GET /merge_requests?state=all GET /merge_requests?milestone=release GET /merge_requests?labels=bug,reproduced GET /merge_requests?author_id=5 +GET /merge_requests?my_reaction_emoji=star GET /merge_requests?scope=assigned-to-me ``` Parameters: -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | -| `state` | string | no | Return all merge requests or just those that are `opened`, `closed`, or `merged`| -| `order_by`| string | no | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` | -| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | -| `milestone` | string | no | Return merge requests for a specific milestone | -| `view` | string | no | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request | -| `labels` | string | no | Return merge requests matching a comma separated list of labels | -| `created_after` | datetime | no | Return merge requests created after the given time (inclusive) | -| `created_before` | datetime | no | Return merge requests created before the given time (inclusive) | -| `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` | -| `author_id` | integer | no | Returns merge requests created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me` | -| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` | +| Attribute | Type | Required | Description | +| ------------------- | -------- | -------- | ---------------------------------------------------------------------------------------------------------------------- | +| `state` | string | no | Return all merge requests or just those that are `opened`, `closed`, or `merged` | +| `order_by` | string | no | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | +| `milestone` | string | no | Return merge requests for a specific milestone | +| `view` | string | no | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request | +| `labels` | string | no | Return merge requests matching a comma separated list of labels | +| `created_after` | datetime | no | Return merge requests created after the given time (inclusive) | +| `created_before` | datetime | no | Return merge requests created before the given time (inclusive) | +| `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` | +| `author_id` | integer | no | Returns merge requests created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me` | +| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` | +| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | ```json [ @@ -116,6 +118,7 @@ GET /projects/:id/merge_requests?state=all GET /projects/:id/merge_requests?iids[]=42&iids[]=43 GET /projects/:id/merge_requests?milestone=release GET /projects/:id/merge_requests?labels=bug,reproduced +GET /projects/:id/merge_requests?my_reaction_emoji=star ``` Parameters: @@ -135,6 +138,7 @@ Parameters: | `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13060] in GitLab 9.5)_ | | `author_id` | integer | no | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | | `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | ```json [ diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 7bcbf9f20ff..56d72d511da 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -40,6 +40,7 @@ module API optional :assignee_id, type: Integer, desc: 'Return merge requests which are assigned to the user with the given ID' optional :scope, type: String, values: %w[created-by-me assigned-to-me all], desc: 'Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all`' + optional :my_reaction_emoji, type: String, desc: 'Return issues reacted by the authenticated user by the given emoji' use :pagination end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 9027090aabd..21d2c9644fb 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -117,6 +117,18 @@ describe API::MergeRequests do expect(json_response.length).to eq(1) expect(json_response.first['id']).to eq(merge_request3.id) end + + it 'returns merge requests reacted by the authenticated user by the given emoji' do + merge_request3 = create(:merge_request, :simple, author: user, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + award_emoji = create(:award_emoji, awardable: merge_request3, user: user2, name: 'star') + + get api('/merge_requests', user2), my_reaction_emoji: award_emoji.name, scope: 'all' + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(1) + expect(json_response.first['id']).to eq(merge_request3.id) + end end end -- cgit v1.2.1 From 785834cfc098c1b74561c49823e2ac061e7d0269 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 5 Sep 2017 09:58:42 +0900 Subject: Fix API document --- doc/api/issues.md | 7 ++++--- doc/api/merge_requests.md | 35 ++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/doc/api/issues.md b/doc/api/issues.md index f8f9db484e2..8ca66049d31 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -41,7 +41,7 @@ GET /issues?my_reaction_emoji=star | `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` _([Introduced][ce-13004] in GitLab 9.5)_ | | `author_id` | integer | no | Return issues created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me`. _([Introduced][ce-13004] in GitLab 9.5)_ | | `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `iids[]` | Array[integer] | no | Return only the issues having the given `iid` | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | @@ -146,7 +146,7 @@ GET /groups/:id/issues?my_reaction_emoji=star | `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | | `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | | `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | | `search` | string | no | Search group issues against their `title` and `description` | @@ -251,7 +251,7 @@ GET /projects/:id/issues?my_reaction_emoji=star | `scope` | string | no | Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13004] in GitLab 9.5)_ | | `author_id` | integer | no | Return issues created by the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | | `assignee_id` | integer | no | Return issues assigned to the given user `id` _([Introduced][ce-13004] in GitLab 9.5)_ | -| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `my_reaction_emoji` | string | no | Return issues reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | | `order_by` | string | no | Return issues ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return issues sorted in `asc` or `desc` order. Default is `desc` | | `search` | string | no | Search project issues against their `title` and `description` | @@ -1099,3 +1099,4 @@ Example response: ``` [ce-13004]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13004 +[ce-14016]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14016 diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index a0b31d3633c..bff8a2d3e4d 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -41,7 +41,7 @@ Parameters: | `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all`. Defaults to `created-by-me` | | `author_id` | integer | no | Returns merge requests created by the given user `id`. Combine with `scope=all` or `scope=assigned-to-me` | | `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` | -| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | ```json [ @@ -123,22 +123,22 @@ GET /projects/:id/merge_requests?my_reaction_emoji=star Parameters: -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | -| `id` | integer | yes | The ID of a project | -| `iids[]` | Array[integer] | no | Return the request having the given `iid` | -| `state` | string | no | Return all merge requests or just those that are `opened`, `closed`, or `merged`| -| `order_by`| string | no | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` | -| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | -| `milestone` | string | no | Return merge requests for a specific milestone | -| `view` | string | no | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request | -| `labels` | string | no | Return merge requests matching a comma separated list of labels | -| `created_after` | datetime | no | Return merge requests created after the given time (inclusive) | -| `created_before` | datetime | no | Return merge requests created before the given time (inclusive) | -| `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13060] in GitLab 9.5)_ | -| `author_id` | integer | no | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | -| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | -| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given emoji ` _([Introduced][ce-14016] in GitLab 10.0)_ | +| Attribute | Type | Required | Description | +| ------------------- | -------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ | +| `id` | integer | yes | The ID of a project | +| `iids[]` | Array[integer] | no | Return the request having the given `iid` | +| `state` | string | no | Return all merge requests or just those that are `opened`, `closed`, or `merged` | +| `order_by` | string | no | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | +| `milestone` | string | no | Return merge requests for a specific milestone | +| `view` | string | no | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request | +| `labels` | string | no | Return merge requests matching a comma separated list of labels | +| `created_after` | datetime | no | Return merge requests created after the given time (inclusive) | +| `created_before` | datetime | no | Return merge requests created before the given time (inclusive) | +| `scope` | string | no | Return merge requests for the given scope: `created-by-me`, `assigned-to-me` or `all` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `author_id` | integer | no | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_ | +| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji` _([Introduced][ce-14016] in GitLab 10.0)_ | ```json [ @@ -1319,3 +1319,4 @@ Example response: ``` [ce-13060]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13060 +[ce-14016]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14016 -- cgit v1.2.1 From dcf09d11447c264f4b4028ea80eea2be913c2f5b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 03:20:54 +0900 Subject: Implement `failure_reason` on `ci_builds` --- app/models/ci/build.rb | 2 +- app/models/commit_status.rb | 18 ++++++++++++++++++ app/services/projects/update_pages_service.rb | 2 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- ...20170830125940_add_failure_reason_to_ci_builds.rb | 9 +++++++++ db/schema.rb | 3 ++- lib/api/commit_statuses.rb | 2 +- lib/api/runner.rb | 2 +- spec/models/ci/build_spec.rb | 20 ++++++++++++++++++++ spec/requests/api/commit_statuses_spec.rb | 3 +++ spec/requests/api/runner_spec.rb | 2 ++ spec/services/projects/update_pages_service_spec.rb | 1 + spec/workers/stuck_ci_jobs_worker_spec.rb | 1 + 13 files changed, 61 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ba3156154ac..9c50d521880 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -103,7 +103,7 @@ module Ci end end - before_transition any => [:failed] do |build| + before_transition any => [:failed] do |build, transition| next if build.retries_max.zero? if build.retries_count < build.retries_max diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 842c6e5cb50..dae3174ba9b 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -38,6 +38,19 @@ class CommitStatus < ActiveRecord::Base scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :after_stage, -> (index) { where('stage_idx > ?', index) } + enum failure_reason: { + no_error: nil, + failed_by_script: 1, # TODO: Not used. Should we expand pipeline as well? + failed_by_missing_dependency: 2, # This will be done in the next MR. + failed_by_system: 3, # TODO: Not used. What's this state? + failed_by_failed_job_state: 4, + failed_by_out_of_quota: 5, # TODO: Only EE. How can we detect? + failed_by_stuck_and_timeout: 6, + failed_by_no_runner: 7, # TODO: Not used. How can we detect? + failed_by_api: 8, + failed_by_page: 9 + } + state_machine :status do event :process do transition [:skipped, :manual] => :created @@ -79,6 +92,11 @@ class CommitStatus < ActiveRecord::Base commit_status.finished_at = Time.now end + before_transition any => :failed do |commit_status, transition| + failure_reason = transition.args.first + commit_status.failure_reason = failure_reason + end + after_transition do |commit_status, transition| next if transition.loopback? diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index f6b83a2f621..c731789ce9b 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -53,7 +53,7 @@ module Projects log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message - @status.drop + @status.drop(:page) super end diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 8b0cfcc8af8..3e635f140fd 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -53,7 +53,7 @@ class StuckCiJobsWorker def drop_build(type, build, status, timeout) Rails.logger.info "#{self.class}: Dropping #{type} build #{build.id} for runner #{build.runner_id} (status: #{status}, timeout: #{timeout})" Gitlab::OptimisticLocking.retry_lock(build, 3) do |b| - b.drop + b.drop(:stuck_and_timeout) end end end diff --git a/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb b/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb new file mode 100644 index 00000000000..5a7487b9227 --- /dev/null +++ b/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb @@ -0,0 +1,9 @@ +class AddFailureReasonToCiBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_builds, :failure_reason, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index a5f867df9ae..216b3cddab0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170824162758) do +ActiveRecord::Schema.define(version: 20170830125940) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -247,6 +247,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.boolean "retried" t.integer "stage_id" t.boolean "protected" + t.integer "failure_reason" end add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 6314ea63197..c129cc9171d 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -103,7 +103,7 @@ module API when 'success' status.success! when 'failed' - status.drop! + status.drop!(:api) when 'canceled' status.cancel! else diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 11999354594..604bfd53296 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -127,7 +127,7 @@ module API when 'success' job.success when 'failed' - job.drop + job.drop(:failed_job_state) end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 3fe3ec17d36..2f39a9b4b0f 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1710,4 +1710,24 @@ describe Ci::Build do end end end + + describe 'set failure_reason when drop' do + let(:build) { create(:ci_build, :created) } + + before do + build.drop!(reason) + end + + context 'when failure_reason is nil' do + let(:reason) { } + + it { expect(build).to be_no_error } + end + + context 'when failure_reason is script_error' do + let(:reason) { :script_error } + + it { expect(build).to be_failed_by_missing_dependency } + end + end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index cc71865e1f3..ce54a5702e3 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -142,6 +142,9 @@ describe API::CommitStatuses do expect(json_response['ref']).not_to be_empty expect(json_response['target_url']).to be_nil expect(json_response['description']).to be_nil + if status == 'failed' + expect(CommitStatus.find(json_response['id'])).to be_failed_by_api + end end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 993164aa8fe..23c2e7aa978 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -627,12 +627,14 @@ describe API::Runner do update_job(state: 'success') expect(job.reload.status).to eq 'success' + expect(job).to be_no_error end it 'mark job as failed' do update_job(state: 'failed') expect(job.reload.status).to eq 'failed' + expect(job).to be_failed_by_failed_job_state end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index aa6ad6340f5..0cc7dde3e4d 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -116,6 +116,7 @@ describe Projects::UpdatePagesService do expect(deploy_status.description) .to match(/artifacts for pages are too large/) + expect(deploy_status).to be_failed_by_page end end diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index 549635f7f33..f0691813e77 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -20,6 +20,7 @@ describe StuckCiJobsWorker do it 'changes status' do worker.perform is_expected.to eq('failed') + expect(job).to be_failed_by_stuck_and_timeout end end -- cgit v1.2.1 From b1af1f268b97c8518bf2806bca48f49174a8aead Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 03:36:15 +0900 Subject: Fix enum wording --- app/models/commit_status.rb | 2 +- app/services/projects/update_pages_service.rb | 2 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- lib/api/commit_statuses.rb | 2 +- spec/requests/api/runner_spec.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index dae3174ba9b..424f8e49d4d 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -43,7 +43,7 @@ class CommitStatus < ActiveRecord::Base failed_by_script: 1, # TODO: Not used. Should we expand pipeline as well? failed_by_missing_dependency: 2, # This will be done in the next MR. failed_by_system: 3, # TODO: Not used. What's this state? - failed_by_failed_job_state: 4, + failed_by_job_state: 4, failed_by_out_of_quota: 5, # TODO: Only EE. How can we detect? failed_by_stuck_and_timeout: 6, failed_by_no_runner: 7, # TODO: Not used. How can we detect? diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index c731789ce9b..0de123515c3 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -53,7 +53,7 @@ module Projects log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message - @status.drop(:page) + @status.drop(:failed_by_page) super end diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 3e635f140fd..0af8090866a 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -53,7 +53,7 @@ class StuckCiJobsWorker def drop_build(type, build, status, timeout) Rails.logger.info "#{self.class}: Dropping #{type} build #{build.id} for runner #{build.runner_id} (status: #{status}, timeout: #{timeout})" Gitlab::OptimisticLocking.retry_lock(build, 3) do |b| - b.drop(:stuck_and_timeout) + b.drop(:failed_by_stuck_and_timeout) end end end diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index c129cc9171d..9ab64452d2b 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -103,7 +103,7 @@ module API when 'success' status.success! when 'failed' - status.drop!(:api) + status.drop!(:failed_by_api) when 'canceled' status.cancel! else diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 23c2e7aa978..48220058c88 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -634,7 +634,7 @@ describe API::Runner do update_job(state: 'failed') expect(job.reload.status).to eq 'failed' - expect(job).to be_failed_by_failed_job_state + expect(job).to be_failed_by_job_state end end -- cgit v1.2.1 From 1d7c0390722c96aa66af5b26f5a826b97293dcd6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 22:03:41 +0900 Subject: Fix enum lists --- app/models/ci/build.rb | 2 +- app/models/commit_status.rb | 14 ++++---------- app/services/projects/update_pages_service.rb | 2 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- lib/api/commit_statuses.rb | 2 +- lib/api/runner.rb | 2 +- spec/models/ci/build_spec.rb | 20 -------------------- spec/models/commit_status_spec.rb | 20 ++++++++++++++++++++ spec/requests/api/commit_statuses_spec.rb | 2 +- spec/requests/api/runner_spec.rb | 4 ++-- spec/services/projects/update_pages_service_spec.rb | 2 +- spec/workers/stuck_ci_jobs_worker_spec.rb | 2 +- 12 files changed, 34 insertions(+), 40 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 9c50d521880..ba3156154ac 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -103,7 +103,7 @@ module Ci end end - before_transition any => [:failed] do |build, transition| + before_transition any => [:failed] do |build| next if build.retries_max.zero? if build.retries_count < build.retries_max diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 424f8e49d4d..1c4088d5af9 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -39,16 +39,10 @@ class CommitStatus < ActiveRecord::Base scope :after_stage, -> (index) { where('stage_idx > ?', index) } enum failure_reason: { - no_error: nil, - failed_by_script: 1, # TODO: Not used. Should we expand pipeline as well? - failed_by_missing_dependency: 2, # This will be done in the next MR. - failed_by_system: 3, # TODO: Not used. What's this state? - failed_by_job_state: 4, - failed_by_out_of_quota: 5, # TODO: Only EE. How can we detect? - failed_by_stuck_and_timeout: 6, - failed_by_no_runner: 7, # TODO: Not used. How can we detect? - failed_by_api: 8, - failed_by_page: 9 + unknown_failure: nil, + job_failure: 1, + api_failure: 2, + stuck_or_timeout_failure: 3, } state_machine :status do diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 0de123515c3..a18b9aae1ea 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -53,7 +53,7 @@ module Projects log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message - @status.drop(:failed_by_page) + @status.drop(:job_failure) super end diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 0af8090866a..269776a1f62 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -53,7 +53,7 @@ class StuckCiJobsWorker def drop_build(type, build, status, timeout) Rails.logger.info "#{self.class}: Dropping #{type} build #{build.id} for runner #{build.runner_id} (status: #{status}, timeout: #{timeout})" Gitlab::OptimisticLocking.retry_lock(build, 3) do |b| - b.drop(:failed_by_stuck_and_timeout) + b.drop(:stuck_or_timeout_failure) end end end diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 9ab64452d2b..829eef18795 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -103,7 +103,7 @@ module API when 'success' status.success! when 'failed' - status.drop!(:failed_by_api) + status.drop!(:api_failure) when 'canceled' status.cancel! else diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 604bfd53296..701c1bff1e0 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -127,7 +127,7 @@ module API when 'success' job.success when 'failed' - job.drop(:failed_job_state) + job.drop(:job_failure) end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 2f39a9b4b0f..3fe3ec17d36 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1710,24 +1710,4 @@ describe Ci::Build do end end end - - describe 'set failure_reason when drop' do - let(:build) { create(:ci_build, :created) } - - before do - build.drop!(reason) - end - - context 'when failure_reason is nil' do - let(:reason) { } - - it { expect(build).to be_no_error } - end - - context 'when failure_reason is script_error' do - let(:reason) { :script_error } - - it { expect(build).to be_failed_by_missing_dependency } - end - end end diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index f7583645e69..4fd330ab7dc 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -443,4 +443,24 @@ describe CommitStatus do end end end + + describe 'set failure_reason when drop' do + let(:build) { create(:ci_build, :created) } + + before do + build.drop!(reason) + end + + context 'when failure_reason is nil' do + let(:reason) { } + + it { expect(build).to be_unknown_failure } + end + + context 'when failure_reason is job_failure' do + let(:reason) { :job_failure } + + it { expect(build).to be_job_failure } + end + end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index ce54a5702e3..e4c73583545 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -143,7 +143,7 @@ describe API::CommitStatuses do expect(json_response['target_url']).to be_nil expect(json_response['description']).to be_nil if status == 'failed' - expect(CommitStatus.find(json_response['id'])).to be_failed_by_api + expect(CommitStatus.find(json_response['id'])).to be_api_failure end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 48220058c88..386e76c6300 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -627,14 +627,14 @@ describe API::Runner do update_job(state: 'success') expect(job.reload.status).to eq 'success' - expect(job).to be_no_error + expect(job).to be_unknown_failure end it 'mark job as failed' do update_job(state: 'failed') expect(job.reload.status).to eq 'failed' - expect(job).to be_failed_by_job_state + expect(job).to be_job_failure end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 0cc7dde3e4d..5fc69c7e4e5 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -116,7 +116,7 @@ describe Projects::UpdatePagesService do expect(deploy_status.description) .to match(/artifacts for pages are too large/) - expect(deploy_status).to be_failed_by_page + expect(deploy_status).to be_job_failure end end diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index f0691813e77..41549a77495 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -20,7 +20,7 @@ describe StuckCiJobsWorker do it 'changes status' do worker.perform is_expected.to eq('failed') - expect(job).to be_failed_by_stuck_and_timeout + expect(job).to be_stuck_or_timeout_failure end end -- cgit v1.2.1 From 68f6c61cf621db82ac98d561782590b1866fcf6f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 16:52:11 +0900 Subject: - Allow runner API to pass failure_reason - Fix spec --- app/models/commit_status.rb | 2 +- .../feature-sm-37239-implement-failure_reason-on-ci_builds.yml | 5 +++++ lib/api/runner.rb | 8 +++++++- spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 + spec/models/commit_status_spec.rb | 10 ++++------ spec/requests/api/commit_statuses_spec.rb | 2 +- spec/requests/api/runner_spec.rb | 9 +++++++++ spec/services/ci/retry_build_service_spec.rb | 2 +- 8 files changed, 29 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 1c4088d5af9..1e8f28b6466 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -42,7 +42,7 @@ class CommitStatus < ActiveRecord::Base unknown_failure: nil, job_failure: 1, api_failure: 2, - stuck_or_timeout_failure: 3, + stuck_or_timeout_failure: 3 } state_machine :status do diff --git a/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml b/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml new file mode 100644 index 00000000000..006b0b45844 --- /dev/null +++ b/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml @@ -0,0 +1,5 @@ +--- +title: Implement `failure_reason` on `ci_builds` +merge_request: 13937 +author: +type: added diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 701c1bff1e0..44ca42fef68 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -114,6 +114,8 @@ module API requires :id, type: Integer, desc: %q(Job's ID) optional :trace, type: String, desc: %q(Job's full trace) optional :state, type: String, desc: %q(Job's status: success, failed) + optional :failure_reason, type: String, values: CommitStatus.failure_reasons.keys, + desc: %q(Job's failure_reason) end put '/:id' do job = authenticate_job! @@ -127,7 +129,11 @@ module API when 'success' job.success when 'failed' - job.drop(:job_failure) + if params[:failure_reason] + job.drop(params[:failure_reason].to_sym) + else + job.drop(:job_failure) + end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 27f2ce60084..b852ac570a3 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -278,6 +278,7 @@ CommitStatus: - auto_canceled_by_id - retried - protected +- failure_reason Ci::Variable: - id - project_id diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 4fd330ab7dc..f5c1db79823 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -445,22 +445,20 @@ describe CommitStatus do end describe 'set failure_reason when drop' do - let(:build) { create(:ci_build, :created) } + let(:commit_status) { create(:commit_status, :created) } - before do - build.drop!(reason) - end + subject { commit_status.drop!(reason); commit_status } context 'when failure_reason is nil' do let(:reason) { } - it { expect(build).to be_unknown_failure } + it { is_expected.to be_unknown_failure } end context 'when failure_reason is job_failure' do let(:reason) { :job_failure } - it { expect(build).to be_job_failure } + it { is_expected.to be_job_failure } end end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index e4c73583545..ed1962ede42 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -143,7 +143,7 @@ describe API::CommitStatuses do expect(json_response['target_url']).to be_nil expect(json_response['description']).to be_nil if status == 'failed' - expect(CommitStatus.find(json_response['id'])).to be_api_failure + expect(json_response['failure_reason']).to eq('api_failure') end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 386e76c6300..f9dcd9d07cb 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -636,6 +636,15 @@ describe API::Runner do expect(job.reload.status).to eq 'failed' expect(job).to be_job_failure end + + context 'when failure_reason is given' do + it 'mark job as failed' do + update_job(state: 'failed', failure_reason: 'stuck_or_timeout_failure') + + expect(job.reload.status).to eq 'failed' + expect(job).to be_stuck_or_timeout_failure + end + end end context 'when tace is given' do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 7c9c117bf71..f5ed9ff608f 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ describe Ci::RetryBuildService do %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried].freeze + user_id auto_canceled_by_id retried failure_reason].freeze shared_examples 'build duplication' do let(:stage) do -- cgit v1.2.1 From e8e8ae4d7df1de71d9f52e774607ac1ba4cce1cc Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 22:01:26 +0900 Subject: Fix spec --- spec/models/commit_status_spec.rb | 5 ++++- spec/requests/api/commit_statuses_spec.rb | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index f5c1db79823..0a6b0023dea 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -447,7 +447,10 @@ describe CommitStatus do describe 'set failure_reason when drop' do let(:commit_status) { create(:commit_status, :created) } - subject { commit_status.drop!(reason); commit_status } + subject do + commit_status.drop!(reason) + commit_status + end context 'when failure_reason is nil' do let(:reason) { } diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index ed1962ede42..e4c73583545 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -143,7 +143,7 @@ describe API::CommitStatuses do expect(json_response['target_url']).to be_nil expect(json_response['description']).to be_nil if status == 'failed' - expect(json_response['failure_reason']).to eq('api_failure') + expect(CommitStatus.find(json_response['id'])).to be_api_failure end end end -- cgit v1.2.1 From 69df47c7e43e1661faf97eb6d2b287a346215c31 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 2 Sep 2017 17:06:25 +0900 Subject: Improve spec --- spec/workers/stuck_ci_jobs_worker_spec.rb | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index 41549a77495..ac6f4fefb4e 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -6,28 +6,31 @@ describe StuckCiJobsWorker do let(:worker) { described_class.new } let(:exclusive_lease_uuid) { SecureRandom.uuid } - subject do - job.reload - job.status - end - before do job.update!(status: status, updated_at: updated_at) allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(exclusive_lease_uuid) end shared_examples 'job is dropped' do - it 'changes status' do + before do worker.perform - is_expected.to eq('failed') + job.reload + end + + it "changes status" do + expect(job).to be_failed expect(job).to be_stuck_or_timeout_failure end end shared_examples 'job is unchanged' do - it "doesn't change status" do + before do worker.perform - is_expected.to eq(status) + job.reload + end + + it "doesn't change status" do + expect(job.status).to eq(status) end end -- cgit v1.2.1 From 5d50cbfaab1d67ffaea6064aeac848f1fc1127a6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 4 Sep 2017 22:46:37 +0900 Subject: Use unknown_failure for runner --- lib/api/runner.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 44ca42fef68..76354fd8c98 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -129,11 +129,8 @@ module API when 'success' job.success when 'failed' - if params[:failure_reason] - job.drop(params[:failure_reason].to_sym) - else - job.drop(:job_failure) - end + failure_reason = params[:failure_reason] ? params[:failure_reason].to_sym : :unknown_failure + job.drop(failure_reason) end end -- cgit v1.2.1 From 38d9b4d77d85e26f827ff9640243494adc8597ed Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 5 Sep 2017 15:10:34 +0900 Subject: Use script_failure. Add runner_system_failure. Improve spec. --- app/models/commit_status.rb | 5 ++-- app/services/projects/update_pages_service.rb | 2 +- lib/api/runner.rb | 3 +-- spec/models/commit_status_spec.rb | 6 ++--- spec/requests/api/runner_spec.rb | 28 +++++++++++++++------- .../services/projects/update_pages_service_spec.rb | 2 +- 6 files changed, 28 insertions(+), 18 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 1e8f28b6466..f3888528940 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -40,9 +40,10 @@ class CommitStatus < ActiveRecord::Base enum failure_reason: { unknown_failure: nil, - job_failure: 1, + script_failure: 1, api_failure: 2, - stuck_or_timeout_failure: 3 + stuck_or_timeout_failure: 3, + runner_system_failure: 4 } state_machine :status do diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index a18b9aae1ea..d34903c9989 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -53,7 +53,7 @@ module Projects log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message - @status.drop(:job_failure) + @status.drop(:script_failure) super end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 76354fd8c98..a3987c560dd 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -129,8 +129,7 @@ module API when 'success' job.success when 'failed' - failure_reason = params[:failure_reason] ? params[:failure_reason].to_sym : :unknown_failure - job.drop(failure_reason) + job.drop(params[:failure_reason] || :unknown_failure) end end diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 0a6b0023dea..858ec831200 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -458,10 +458,10 @@ describe CommitStatus do it { is_expected.to be_unknown_failure } end - context 'when failure_reason is job_failure' do - let(:reason) { :job_failure } + context 'when failure_reason is script_failure' do + let(:reason) { :script_failure } - it { is_expected.to be_job_failure } + it { is_expected.to be_script_failure } end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index f9dcd9d07cb..419a65c69b5 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -626,24 +626,34 @@ describe API::Runner do it 'mark job as succeeded' do update_job(state: 'success') - expect(job.reload.status).to eq 'success' - expect(job).to be_unknown_failure + job.reload + expect(job).to be_success end it 'mark job as failed' do update_job(state: 'failed') - expect(job.reload.status).to eq 'failed' - expect(job).to be_job_failure + job.reload + expect(job).to be_failed + expect(job).to be_unknown_failure end - context 'when failure_reason is given' do - it 'mark job as failed' do - update_job(state: 'failed', failure_reason: 'stuck_or_timeout_failure') + context 'when failure_reason is script_failure' do + before do + update_job(state: 'failed', failure_reason: 'script_failure') + job.reload + end + + it { expect(job).to be_script_failure } + end - expect(job.reload.status).to eq 'failed' - expect(job).to be_stuck_or_timeout_failure + context 'when failure_reason is runner_system_failure' do + before do + update_job(state: 'failed', failure_reason: 'runner_system_failure') + job.reload end + + it { expect(job).to be_runner_system_failure } end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 5fc69c7e4e5..031366d1825 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -116,7 +116,7 @@ describe Projects::UpdatePagesService do expect(deploy_status.description) .to match(/artifacts for pages are too large/) - expect(deploy_status).to be_job_failure + expect(deploy_status).to be_script_failure end end -- cgit v1.2.1 From 9f037034a9a74901f5fa05a5281a4127d8a46d24 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 5 Sep 2017 07:39:52 +0000 Subject: Adds documentation to use Vuex --- doc/development/fe_guide/vue.md | 291 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 288 insertions(+), 3 deletions(-) diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 0742b202807..2607353782a 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -28,8 +28,9 @@ As always, the Frontend Architectural Experts are available to help with any Vue All new features built with Vue.js must follow a [Flux architecture][flux]. The main goal we are trying to achieve is to have only one data flow and only one data entry. -In order to achieve this goal, each Vue bundle needs a Store - where we keep all the data -, -a Service - that we use to communicate with the server - and a main Vue component. +In order to achieve this goal, you can either use [vuex](#vuex) or use the [store pattern][state-management], explained below: + +Each Vue bundle needs a Store - where we keep all the data -,a Service - that we use to communicate with the server - and a main Vue component. Think of the Main Vue Component as the entry point of your application. This is the only smart component that should exist in each Vue feature. @@ -74,6 +75,59 @@ provided as a prop to the main component. Don't forget to follow [these steps.][page_specific_javascript] +### Bootstrapping Gotchas +#### Providing data from Haml to JavaScript +While mounting a Vue application may be a need to provide data from Rails to JavaScript. +To do that, provide the data through `data` attributes in the HTML element and query them while mounting the application. + +_Note:_ You should only do this while initing the application, because the mounted element will be replaced with Vue-generated DOM. + +The advantage of providing data from the DOM to the Vue instance through `props` in the `render` function +instead of querying the DOM inside the main vue component is that makes tests easier by avoiding the need to +create a fixture or an HTML element in the unit test. See the following example: + +```javascript +// haml +.js-vue-app{ data: { endpoint: 'foo' }} + +document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '.js-vue-app', + data() { + const dataset = this.$options.el.dataset; + return { + endpoint: dataset.endpoint, + }; + }, + render(createElement) { + return createElement('my-component', { + props: { + endpoint: this.isLoading, + }, + }); + }, +})); +``` + +#### Accessing the `gl` object +When we need to query the `gl` object for data that won't change during the application's lyfecyle, we should do it in the same place where we query the DOM. +By following this practice, we can avoid the need to mock the `gl` object, which will make tests easier. +It should be done while initializing our Vue instance, and the data should be provided as `props` to the main component: + +##### example: +```javascript + +document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '.js-vue-app', + render(createElement) { + return createElement('my-component', { + props: { + username: gon.current_username, + }, + }); + }, +})); +``` + ### A folder for Components This folder holds all components that are specific of this new feature. @@ -89,6 +143,29 @@ in one table would not be a good use of this pattern. You can read more about components in Vue.js site, [Component System][component-system] +#### Components Gotchas +1. Using SVGs in components: To use an SVG in a template we need to make it a property we can access through the component. +A `prop` and a property returned by the `data` functions require `vue` to set a `getter` and a `setter` for each of them. +The SVG should be a computed property in order to improve performance, note that computed properties are cached based on their dependencies. + +```javascript +// bad +import svg from 'svg.svg'; +data() { + return { + myIcon: svg, + }; +}; + +// good +import svg from 'svg.svg'; +computed: { + myIcon() { + return svg; + } +} +``` + ### A folder for the Store The Store is a class that allows us to manage the state in a single @@ -430,11 +507,23 @@ describe('Todos App', () => { }); }); ``` +#### `mountComponent` helper +There is an helper in `spec/javascripts/helpers/vue_mount_component_helper.js` that allows you to mount a component with the given props: + +```javascript +import Vue from 'vue'; +import mountComponent from 'helpers/vue_mount_component_helper.js' +import component from 'component.vue' + +const Component = Vue.extend(component); +const data = {prop: 'foo'}; +const vm = mountComponent(Component, data); +``` + #### Test the component's output The main return value of a Vue component is the rendered output. In order to test the component we need to test the rendered output. [Vue][vue-test] guide's to unit test show us exactly that: - ### Stubbing API responses [Vue Resource Interceptors][vue-resource-interceptor] allow us to add a interceptor with the response we need: @@ -481,6 +570,198 @@ new Component({ new Component().$mount(); ``` +## Vuex +To manage the state of an application you may use [Vuex][vuex-docs]. + +_Note:_ All of the below is explained in more detail in the official [Vuex documentation][vuex-docs]. + +### Separation of concerns +Vuex is composed of State, Getters, Mutations, Actions and Modules. + +When a user clicks on an action, we need to `dispatch` it. This action will `commit` a mutation that will change the state. +_Note:_ The action itself will not update the state, only a mutation should update the state. + +#### File structure +When using Vuex at GitLab, separate this concerns into different files to improve readability. If you can, separate the Mutation Types as well: + +``` +└── store + ├── index.js # where we assemble modules and export the store + ├── actions.js # actions + ├── mutations.js # mutations + ├── getters.js # getters + └── mutation_types.js # mutation types +``` +The following examples show an application that lists and adds users to the state. + +##### `index.js` +This is the entry point for our store. You can use the following as a guide: + +```javascript +import Vue from 'vue'; +import Vuex from 'vuex'; +import * as actions from './actions'; +import * as mutations from './mutations'; + +Vue.use(Vuex); + +export default new Vuex.Store({ + actions, + getters, + state: { + users: [], + }, +}); +``` +_Note:_ If the state of the application is too complex, an individual file for the state may be better. + +#### `actions.js` +An action commits a mutatation. In this file, we will write the actions that will call the respective mutation: + +```javascript + import * as types from './mutation-types' + + export const addUser = ({ commit }, user) => { + commit(types.ADD_USER, user); + }; +``` + +To dispatch an action from a component, use the `mapActions` helper: +```javascript +import { mapActions } from 'vuex'; + +{ + methods: { + ...mapActions([ + 'addUser', + ]), + onClickUser(user) { + this.addUser(user); + }, + }, +}; +``` + +#### `getters.js` +Sometimes we may need to get derived state based on store state, like filtering for a specific prop. This can be done through the `getters`: + +```javascript +// get all the users with pets +export getUsersWithPets = (state, getters) => { + return state.users.filter(user => user.pet !== undefined); +}; +``` + +To access a getter from a component, use the `mapGetters` helper: +```javascript +import { mapGetters } from 'vuex'; + +{ + computed: { + ...mapGetters([ + 'getUsersWithPets', + ]), + }, +}; +``` + +#### `mutations.js` +The only way to actually change state in a Vuex store is by committing a mutation. + +```javascript + import * as types from './mutation-types' + export default { + [types.ADD_USER](state, user) { + state.users.push(user); + }, + }; +``` + +#### `mutations_types.js` +From [vuex mutations docs][vuex-mutations]: +> It is a commonly seen pattern to use constants for mutation types in various Flux implementations. This allows the code to take advantage of tooling like linters, and putting all constants in a single file allows your collaborators to get an at-a-glance view of what mutations are possible in the entire application. + +```javascript +export const ADD_USER = 'ADD_USER'; +``` + +### How to include the store in your application +The store should be included in the main component of your application: +```javascript + // app.vue + import store from 'store'; // it will include the index.js file + + export default { + name: 'application', + store, + ... + }; +``` + +### Vuex Gotchas +1. Avoid calling a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency through out the application. From Vuex docs: + + > why don't we just call store.commit('action') directly? Well, remember that mutations must be synchronous? Actions aren't. We can perform asynchronous operations inside an action. + + ```javascript + // component.vue + + // bad + created() { + this.$store.commit('mutation'); + } + + // good + created() { + this.$store.dispatch('action'); + } + ``` +1. When possible, use mutation types instead of hardcoding strings. It will be less error prone. +1. The State will be accessible in all components descending from the use where the store is instantiated. + +### Testing Vuex +#### Testing Vuex concerns +Refer to [vuex docs][vuex-testing] regarding testing Actions, Getters and Mutations. + +#### Testing components that need a store +Smaller components might use `store` properties to access the data. +In order to write unit tests for those components, we need to include the store and provide the correct state: + +```javascript +//component_spec.js +import Vue from 'vue'; +import store from './store'; +import component from './component.vue' + +describe('component', () => { + let vm; + let Component; + + beforeEach(() => { + Component = Vue.extend(issueActions); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should show a user', () => { + const user = { + name: 'Foo', + age: '30', + }; + + // populate the store + store.dipatch('addUser', user); + + vm = new Component({ + store, + propsData: props, + }).$mount(); + }); +}); +``` + [vue-docs]: http://vuejs.org/guide/index.html [issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards [environments-table]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/environments @@ -493,3 +774,7 @@ new Component().$mount(); [vue-test]: https://vuejs.org/v2/guide/unit-testing.html [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 [flux]: https://facebook.github.io/flux +[vuex-docs]: https://vuex.vuejs.org +[vuex-structure]: https://vuex.vuejs.org/en/structure.html +[vuex-mutations]: https://vuex.vuejs.org/en/mutations.html +[vuex-testing]: https://vuex.vuejs.org/en/testing.html -- cgit v1.2.1 From c47e1c3975255774ebef69f5ce58b43ddda12065 Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Mon, 4 Sep 2017 21:11:55 +0530 Subject: Make query param `simple` part of config --- app/assets/javascripts/api.js | 3 ++- spec/javascripts/api_spec.js | 6 ++++-- spec/javascripts/project_title_spec.js | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index 78cb3def879..8acddd6194c 100644 --- a/app/assets/javascripts/api.js +++ b/app/assets/javascripts/api.js @@ -5,7 +5,7 @@ const Api = { groupPath: '/api/:version/groups/:id.json', namespacesPath: '/api/:version/namespaces.json', groupProjectsPath: '/api/:version/groups/:id/projects.json', - projectsPath: '/api/:version/projects.json?simple=true', + projectsPath: '/api/:version/projects.json', labelsPath: '/:namespace_path/:project_path/labels', licensePath: '/api/:version/templates/licenses/:key', gitignorePath: '/api/:version/templates/gitignores/:key', @@ -58,6 +58,7 @@ const Api = { const defaults = { search: query, per_page: 20, + simple: true, }; if (gon.current_user_id) { diff --git a/spec/javascripts/api_spec.js b/spec/javascripts/api_spec.js index 8c68ceff914..2aa4fb1f6c6 100644 --- a/spec/javascripts/api_spec.js +++ b/spec/javascripts/api_spec.js @@ -101,12 +101,13 @@ describe('Api', () => { it('fetches projects with membership when logged in', (done) => { const query = 'dummy query'; const options = { unused: 'option' }; - const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json?simple=true`; + const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json`; window.gon.current_user_id = 1; const expectedData = Object.assign({ search: query, per_page: 20, membership: true, + simple: true, }, options); spyOn(jQuery, 'ajax').and.callFake((request) => { expect(request.url).toEqual(expectedUrl); @@ -124,10 +125,11 @@ describe('Api', () => { it('fetches projects without membership when not logged in', (done) => { const query = 'dummy query'; const options = { unused: 'option' }; - const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json?simple=true`; + const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json`; const expectedData = Object.assign({ search: query, per_page: 20, + simple: true, }, options); spyOn(jQuery, 'ajax').and.callFake((request) => { expect(request.url).toEqual(expectedUrl); diff --git a/spec/javascripts/project_title_spec.js b/spec/javascripts/project_title_spec.js index 3d36bb3e4d4..9ea8414ae40 100644 --- a/spec/javascripts/project_title_spec.js +++ b/spec/javascripts/project_title_spec.js @@ -41,12 +41,13 @@ describe('Project Title', () => { window.gon.current_user_id = 1; $('.js-projects-dropdown-toggle').click(); expect($menu).toHaveClass('open'); - expect(reqUrl).toBe(`/api/${dummyApiVersion}/projects.json?simple=true`); + expect(reqUrl).toBe(`/api/${dummyApiVersion}/projects.json`); expect(reqData).toEqual({ search: '', order_by: 'last_activity_at', per_page: 20, membership: true, + simple: true, }); $menu.find('.dropdown-menu-close-icon').click(); expect($menu).not.toHaveClass('open'); -- cgit v1.2.1 From 8c70224d1829a81567b0edf86f2fd9d7b94c0298 Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Mon, 4 Sep 2017 21:12:27 +0530 Subject: Add support for `sizeClass`, defaults to `s40` --- .../vue_shared/components/identicon.vue | 8 ++++++- .../vue_shared/components/identicon_spec.js | 28 +++++++++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/vue_shared/components/identicon.vue b/app/assets/javascripts/vue_shared/components/identicon.vue index 0edd820743f..7cf2e029cf6 100644 --- a/app/assets/javascripts/vue_shared/components/identicon.vue +++ b/app/assets/javascripts/vue_shared/components/identicon.vue @@ -9,6 +9,11 @@ export default { type: String, required: true, }, + sizeClass: { + type: String, + required: false, + default: 's40', + }, }, computed: { /** @@ -38,7 +43,8 @@ export default {