Merge branch 'lm-add-deprecation-warning-inf' into 'master'

Implement warnings for deprecated keywords

See merge request gitlab-org/gitlab!76679

app/graphql/resolvers/ci/config_resolver.rb

@@ -47,11 +47,13 @@ module Resolvers
           {
             status: :valid,
             errors: [],
+            warnings: result.warnings,
             stages: make_stages(result.jobs)
           }
         else
           {
             status: :invalid,
+            warnings: result.warnings,
             errors: result.errors
           }
         end

app/graphql/types/ci/config/config_type.rb

@@ -15,6 +15,8 @@ module Types
               description: 'Stages of the pipeline.'
         field :status, Types::Ci::Config::StatusEnum, null: true,
               description: 'Status of linting, can be either valid or invalid.'
+        field :warnings, [GraphQL::Types::String], null: true,
+              description: 'Linting warnings.'
       end
     end
   end

app/models/users/callout.rb

@@ -40,7 +40,8 @@ module Users
       profile_personal_access_token_expiry: 37,  # EE-only
       terraform_notification_dismissed: 38,
       security_newsletter_callout: 39,
-      verification_reminder: 40                  # EE-only
+      verification_reminder: 40,                  # EE-only
+      ci_deprecation_warning_for_types_keyword: 41
     }
 
     validates :feature_name,

doc/api/graphql/reference/index.md

@@ -8754,6 +8754,7 @@ Represents the total number of issues and their weights for a particular day.
 | <a id="ciconfigmergedyaml"></a>`mergedYaml` | [`String`](#string) | Merged CI configuration YAML. |
 | <a id="ciconfigstages"></a>`stages` | [`CiConfigStageConnection`](#ciconfigstageconnection) | Stages of the pipeline. (see [Connections](#connections)) |
 | <a id="ciconfigstatus"></a>`status` | [`CiConfigStatus`](#ciconfigstatus) | Status of linting, can be either valid or invalid. |
+| <a id="ciconfigwarnings"></a>`warnings` | [`[String!]`](#string) | Linting warnings. |
 
 ### `CiConfigGroup`
 
@@ -17434,6 +17435,7 @@ Name of the feature that the callout is for.
 | <a id="usercalloutfeaturenameenumactive_user_count_threshold"></a>`ACTIVE_USER_COUNT_THRESHOLD` | Callout feature name for active_user_count_threshold. |
 | <a id="usercalloutfeaturenameenumbuy_pipeline_minutes_notification_dot"></a>`BUY_PIPELINE_MINUTES_NOTIFICATION_DOT` | Callout feature name for buy_pipeline_minutes_notification_dot. |
 | <a id="usercalloutfeaturenameenumcanary_deployment"></a>`CANARY_DEPLOYMENT` | Callout feature name for canary_deployment. |
+| <a id="usercalloutfeaturenameenumci_deprecation_warning_for_types_keyword"></a>`CI_DEPRECATION_WARNING_FOR_TYPES_KEYWORD` | Callout feature name for ci_deprecation_warning_for_types_keyword. |
 | <a id="usercalloutfeaturenameenumcloud_licensing_subscription_activation_banner"></a>`CLOUD_LICENSING_SUBSCRIPTION_ACTIVATION_BANNER` | Callout feature name for cloud_licensing_subscription_activation_banner. |
 | <a id="usercalloutfeaturenameenumcluster_security_warning"></a>`CLUSTER_SECURITY_WARNING` | Callout feature name for cluster_security_warning. |
 | <a id="usercalloutfeaturenameenumeoa_bronze_plan_banner"></a>`EOA_BRONZE_PLAN_BANNER` | Callout feature name for eoa_bronze_plan_banner. |

lib/gitlab/ci/config.rb

@@ -36,7 +36,7 @@ module Gitlab
         end
 
         @root = self.logger.instrument(:config_compose) do
-          Entry::Root.new(@config).tap(&:compose!)
+          Entry::Root.new(@config, project: project, user: user).tap(&:compose!)
         end
       rescue *rescue_errors => e
         raise Config::ConfigError, e.message

lib/gitlab/ci/config/entry/root.rb

@@ -59,7 +59,8 @@ module Gitlab
 
           entry :types, Entry::Stages,
             description: 'Deprecated: stages for this pipeline.',
-            reserved: true
+            reserved: true,
+            deprecation: { deprecated: '9.0', warning: '14.8', removed: '15.0', documentation: 'https://docs.gitlab.com/ee/ci/yaml/#deprecated-keywords' }
 
           entry :cache, Entry::Caches,
             description: 'Configure caching between build jobs.',
@@ -122,8 +123,9 @@ module Gitlab
             # Deprecated `:types` key workaround - if types are defined and
             # stages are not defined we use types definition as stages.
             #
-            if types_defined? && !stages_defined?
-              @entries[:stages] = @entries[:types]
+            if types_defined?
+              @entries[:stages] = @entries[:types] unless stages_defined?
+              log_and_warn_deprecated_entry(@entries[:types])
             end
 
             @entries.delete(:types)

lib/gitlab/config/entry/configurable.rb

@@ -76,7 +76,7 @@ module Gitlab
           private
 
           # rubocop: disable CodeReuse/ActiveRecord
-          def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil, metadata: {})
+          def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil, deprecation: nil, metadata: {})
             entry_name = key.to_sym
             raise ArgumentError, "Entry '#{key}' already defined in '#{name}'" if @nodes.to_h[entry_name]
 
@@ -85,6 +85,7 @@ module Gitlab
               .with(default: default)
               .with(inherit: inherit)
               .with(reserved: reserved)
+              .with(deprecation: deprecation)
               .metadata(metadata)
 
             @nodes ||= {}

lib/gitlab/config/entry/factory.rb

@@ -32,6 +32,10 @@ module Gitlab
           self
         end
 
+        def deprecation
+          @attributes[:deprecation]
+        end
+
         def description
           @attributes[:description]
         end
@@ -84,6 +88,7 @@ module Gitlab
             node.parent = @attributes[:parent]
             node.default = @attributes[:default]
             node.description = @attributes[:description]
+            node.deprecation = @attributes[:deprecation]
           end
         end
       end

lib/gitlab/config/entry/node.rb

@@ -10,7 +10,7 @@ module Gitlab
         InvalidError = Class.new(StandardError)
 
         attr_reader :config, :metadata
-        attr_accessor :key, :parent, :default, :description
+        attr_accessor :key, :parent, :default, :description, :deprecation
 
         def initialize(config, **metadata)
           @config = config
@@ -128,6 +128,25 @@ module Gitlab
         private
 
         attr_reader :entries
+
+        def log_and_warn_deprecated_entry(entry)
+          user = metadata[:user]
+          project = metadata[:project]
+
+          if project && user
+            Gitlab::AppJsonLogger.info(event: 'ci_used_deprecated_keyword',
+                                       entry: entry.key.to_s,
+                                       user_id: user.id,
+                                       project_id: project.id)
+          end
+
+          deprecation = entry.deprecation
+          add_warning(
+            "`#{entry.key}` is deprecated in " \
+            "#{deprecation[:deprecated]} and will be removed in #{deprecation[:removed]} " \
+            "- read more: #{deprecation[:documentation]}"
+          )
+        end
       end
     end
   end

spec/graphql/types/ci/config/config_type_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe Types::Ci::Config::ConfigType do
       mergedYaml
       stages
       status
+      warnings
     ]
 
     expect(described_class).to have_graphql_fields(*expected_fields)

spec/lib/gitlab/ci/config/entry/root_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Config::Entry::Root do
-  let(:root) { described_class.new(hash) }
+  let(:user) {}
+  let(:project) {}
+  let(:root) { described_class.new(hash, user: user, project: project) }
 
   describe '.nodes' do
     it 'returns a hash' do
@@ -53,6 +55,37 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
         }
       end
 
+      context 'when deprecated types keyword is defined' do
+        let(:project) { create(:project, :repository) }
+        let(:user) { create(:user) }
+
+        let(:hash) do
+          { types: %w(test deploy),
+            rspec: { script: 'rspec' } }
+        end
+
+        before do
+          root.compose!
+        end
+
+        it 'returns array of types as stages with a warning' do
+          expect(root.stages_value).to eq %w[test deploy]
+          expect(root.warnings).to eq(["root `types` is deprecated in 9.0 and will be removed in 15.0 - read more: https://docs.gitlab.com/ee/ci/yaml/#deprecated-keywords"])
+        end
+
+        it 'logs usage of types keyword' do
+          expect(Gitlab::AppJsonLogger).to(
+            receive(:info)
+              .with(event: 'ci_used_deprecated_keyword',
+                    entry: root[:stages].key.to_s,
+                    user_id: user.id,
+                    project_id: project.id)
+          )
+
+          root.compose!
+        end
+      end
+
       describe '#compose!' do
         before do
           root.compose!
@@ -108,17 +141,6 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
               expect(root.stages_value).to eq %w[build pages release]
             end
           end
-
-          context 'when deprecated types key defined' do
-            let(:hash) do
-              { types: %w(test deploy),
-                rspec: { script: 'rspec' } }
-            end
-
-            it 'returns array of types as stages' do
-              expect(root.stages_value).to eq %w[test deploy]
-            end
-          end
         end
 
         describe '#jobs_value' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -2608,7 +2608,7 @@ module Gitlab
         end
 
         context 'returns errors if job stage is not a defined stage' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", type: "acceptance" } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", type: "acceptance" } }) }
 
           it_behaves_like 'returns errors', 'rspec job: chosen stage does not exist; available stages are .pre, build, test, .post'
         end
@@ -2644,37 +2644,37 @@ module Gitlab
         end
 
         context 'returns errors if job artifacts:name is not an a string' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { name: 1 } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { name: 1 } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts name should be a string'
         end
 
         context 'returns errors if job artifacts:when is not an a predefined value' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { when: 1 } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { when: 1 } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts when should be on_success, on_failure or always'
         end
 
         context 'returns errors if job artifacts:expire_in is not an a string' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { expire_in: 1 } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { expire_in: 1 } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts expire in should be a duration'
         end
 
         context 'returns errors if job artifacts:expire_in is not an a valid duration' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { expire_in: "7 elephants" } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { expire_in: "7 elephants" } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts expire in should be a duration'
         end
 
         context 'returns errors if job artifacts:untracked is not an array of strings' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { untracked: "string" } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { untracked: "string" } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts untracked should be a boolean value'
         end
 
         context 'returns errors if job artifacts:paths is not an array of strings' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", artifacts: { paths: "string" } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", artifacts: { paths: "string" } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:artifacts paths should be an array of strings'
         end
@@ -2698,49 +2698,49 @@ module Gitlab
         end
 
         context 'returns errors if job cache:key is not an a string' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { key: 1 } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { key: 1 } } }) }
 
           it_behaves_like 'returns errors', "jobs:rspec:cache:key should be a hash, a string or a symbol"
         end
 
         context 'returns errors if job cache:key:files is not an array of strings' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { key: { files: [1] } } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { key: { files: [1] } } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:cache:key:files config should be an array of strings'
         end
 
         context 'returns errors if job cache:key:files is an empty array' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { key: { files: [] } } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { key: { files: [] } } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:cache:key:files config requires at least 1 item'
         end
 
         context 'returns errors if job defines only cache:key:prefix' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { key: { prefix: 'prefix-key' } } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { key: { prefix: 'prefix-key' } } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:cache:key config missing required keys: files'
         end
 
         context 'returns errors if job cache:key:prefix is not an a string' do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { key: { prefix: 1, files: ['file'] } } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { key: { prefix: 1, files: ['file'] } } } }) }
 
           it_behaves_like 'returns errors', 'jobs:rspec:cache:key:prefix config should be a string or symbol'
         end
 
         context "returns errors if job cache:untracked is not an array of strings" do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { untracked: "string" } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { untracked: "string" } } }) }
 
           it_behaves_like 'returns errors', "jobs:rspec:cache:untracked config should be a boolean value"
         end
 
         context "returns errors if job cache:paths is not an array of strings" do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", cache: { paths: "string" } } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", cache: { paths: "string" } } }) }
 
           it_behaves_like 'returns errors', "jobs:rspec:cache:paths config should be an array of strings"
         end
 
         context "returns errors if job dependencies is not an array of strings" do
-          let(:config) { YAML.dump({ types: %w(build test), rspec: { script: "test", dependencies: "string" } }) }
+          let(:config) { YAML.dump({ stages: %w(build test), rspec: { script: "test", dependencies: "string" } }) }
 
           it_behaves_like 'returns errors', "jobs:rspec dependencies should be an array of strings"
         end

spec/lib/gitlab/config/entry/configurable_spec.rb

@@ -39,7 +39,8 @@ RSpec.describe Gitlab::Config::Entry::Configurable do
         entry :object, entry_class,
           description: 'test object',
           inherit: true,
-          reserved: true
+          reserved: true,
+          deprecation: { deprecated: '10.0', warning: '10.1', removed: '11.0', documentation: 'docs.gitlab.com' }
       end
     end
 
@@ -52,6 +53,12 @@ RSpec.describe Gitlab::Config::Entry::Configurable do
         factory = entry.nodes[:object]
 
         expect(factory).to be_an_instance_of(Gitlab::Config::Entry::Factory)
+        expect(factory.deprecation).to eq(
+          deprecated: '10.0',
+          warning: '10.1',
+          removed: '11.0',
+          documentation: 'docs.gitlab.com'
+        )
         expect(factory.description).to eq('test object')
         expect(factory.inheritable?).to eq(true)
         expect(factory.reserved?).to eq(true)

spec/lib/gitlab/config/entry/factory_spec.rb

@@ -115,5 +115,16 @@ RSpec.describe Gitlab::Config::Entry::Factory do
           .with('some value', { some: 'hash' })
       end
     end
+
+    context 'when setting deprecation information' do
+      it 'passes deprecation as a parameter' do
+        entry = factory
+           .value('some value')
+           .with(deprecation: { deprecated: '10.0', warning: '10.1', removed: '11.0', documentation: 'docs' })
+           .create!
+
+        expect(entry.deprecation).to eq({ deprecated: '10.0', warning: '10.1', removed: '11.0', documentation: 'docs' })
+      end
+    end
   end
 end

spec/requests/api/graphql/ci/config_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe 'Query.ciConfig' do
         ciConfig(projectPath: "#{project.full_path}", content: "#{content}", dryRun: false) {
           status
           errors
+          warnings
           stages {
             nodes {
               name
@@ -73,6 +74,7 @@ RSpec.describe 'Query.ciConfig' do
     expect(graphql_data['ciConfig']).to eq(
       "status" => "VALID",
       "errors" => [],
+      "warnings" => [],
       "stages" =>
       {
         "nodes" =>
@@ -220,6 +222,21 @@ RSpec.describe 'Query.ciConfig' do
     )
   end
 
+  context 'when using deprecated keywords' do
+    let_it_be(:content) do
+      YAML.dump(
+        rspec: { script: 'ls' },
+        types: ['test']
+      )
+    end
+
+    it 'returns a warning' do
+      post_graphql_query
+
+      expect(graphql_data['ciConfig']['warnings']).to include('root `types` is deprecated in 9.0 and will be removed in 15.0 - read more: https://docs.gitlab.com/ee/ci/yaml/#deprecated-keywords')
+    end
+  end
+
   context 'when the config file includes other files' do
     let_it_be(:content) do
       YAML.dump(
@@ -250,6 +267,7 @@ RSpec.describe 'Query.ciConfig' do
       expect(graphql_data['ciConfig']).to eq(
         "status" => "VALID",
         "errors" => [],
+        "warnings" => [],
         "stages" =>
         {
           "nodes" =>

spec/requests/api/lint_spec.rb

@@ -121,8 +121,8 @@ RSpec.describe API::Lint do
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response).to be_an Hash
           expect(json_response['status']).to eq('valid')
-          expect(json_response['warnings']).to eq([])
-          expect(json_response['errors']).to eq([])
+          expect(json_response['warnings']).to match_array([])
+          expect(json_response['errors']).to match_array([])
         end
 
         it 'outputs expanded yaml content' do
@@ -149,7 +149,20 @@ RSpec.describe API::Lint do
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response['status']).to eq('valid')
           expect(json_response['warnings']).not_to be_empty
-          expect(json_response['errors']).to eq([])
+          expect(json_response['errors']).to match_array([])
+        end
+      end
+
+      context 'with valid .gitlab-ci.yaml using deprecated keywords' do
+        let(:yaml_content) { { job: { script: 'ls' }, types: ['test'] }.to_yaml }
+
+        it 'passes validation but returns warnings' do
+          post api('/ci/lint', api_user), params: { content: yaml_content }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['status']).to eq('valid')
+          expect(json_response['warnings']).not_to be_empty
+          expect(json_response['errors']).to match_array([])
         end
       end
 

spec/support/gitlab_stubs/gitlab_ci.yml

@@ -9,7 +9,7 @@ before_script:
 variables:
   DB_NAME: postgres
 
-types:
+stages:
   - test
   - deploy
   - notify
@@ -36,7 +36,7 @@ staging:
     KEY1: value1
     KEY2: value2
   script: "cap deploy stating"
-  type: deploy
+  stage: deploy
   tags:
     - ruby
     - mysql
@@ -46,7 +46,7 @@ staging:
 production:
   variables:
     DB_NAME: mysql
-  type: deploy
+  stage: deploy
   script:
     - cap deploy production
     - cap notify
@@ -58,7 +58,7 @@ production:
     - /^deploy-.*$/
 
 dockerhub:
-  type: notify
+  stage: notify
   script: "curl http://dockerhub/URL"
   tags:
     - ruby