Merge branch 'mg-improve-ci-lint-api-20240129' into 'master'

Use less ambiguous parameter names in CI lint API

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143098



Merged-by: Rémy Coutable <remy@rymai.me>
Approved-by: Leaminn Ma <lma@gitlab.com>
Approved-by: Rémy Coutable <remy@rymai.me>
Reviewed-by: Rémy Coutable <remy@rymai.me>
Reviewed-by: Furkan Ayhan <furkanayhn@gmail.com>
Reviewed-by: Marcel Amirault <mamirault@gitlab.com>
Reviewed-by: Leaminn Ma <lma@gitlab.com>
Co-authored-by: Manuel Grabowski <mgrabowski@gitlab.com>

doc/api/lint.md

@@ -61,9 +61,10 @@ Example responses:
 ## Validate a project's CI configuration
 
 > - `sha` attribute [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/369212) in GitLab 16.5.
+> - `sha` and `ref` [renamed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143098) to `content_ref` and `dry_run_ref` in GitLab 16.10.
 
-Checks if a project’s `.gitlab-ci.yml` configuration in a given ref (the
-`sha` parameter, by default `HEAD` of the project’s default branch) is valid.
+Checks if a project's `.gitlab-ci.yml` configuration in a given ref (the
+`content_ref` parameter, by default `HEAD` of the project's default branch) is valid.
 This endpoint uses all namespace specific data available, including variables
 and local includes.
 
@@ -73,10 +74,12 @@ GET /projects/:id/ci/lint
 
 | Attribute      | Type    | Required | Description |
 |----------------|---------|----------|-------------|
+| `content_ref`  | string  | No       | The CI/CD configuration content is taken from this commit SHA, branch or tag. Defaults to the SHA of the head of the project's default branch when not set. |
+| `dry_run_ref`  | string  | No       | When `dry_run` is `true`, sets the branch or tag context to use to validate the CI/CD YAML configuration. Defaults to the project's default branch when not set. |
 | `dry_run`      | boolean | No       | Run pipeline creation simulation, or only do static check. |
 | `include_jobs` | boolean | No       | If the list of jobs that would exist in a static check or pipeline simulation should be included in the response. Default: `false`. |
-| `ref`          | string  | No       | When `dry_run` is `true`, sets the branch or tag context to use to validate the CI/CD YAML configuration. Defaults to the project's default branch when not set. |
-| `sha`          | string  | No       | The CI/CD configuration content is taken from this commit SHA, branch or tag. Defaults to the SHA of the head of the project's default branch when not set. |
+| `ref`          | string  | No       | (Deprecated) When `dry_run` is `true`, sets the branch or tag context to use to validate the CI/CD YAML configuration. Defaults to the project's default branch when not set. Use `dry_run_ref` instead. |
+| `sha`          | string  | No       | (Deprecated) The CI/CD configuration content is taken from this commit SHA, branch or tag. Defaults to the SHA of the head of the project's default branch when not set. Use `content_ref` instead. |
 
 Example request:
 

lib/api/lint.rb

@@ -15,24 +15,32 @@ class Lint < ::API::Base
         ]
       end
       params do
-        optional :sha, type: String, desc: 'The commit hash or name of a repository branch or tag. Defaults to the HEAD of the project’s default branch'
+        optional :sha, type: String, desc: 'Deprecated: Use content_ref instead'
+        optional :content_ref, type: String, desc: "The CI/CD configuration content is taken from this commit SHA, branch or tag. Defaults to the HEAD of the project's default branch"
+        mutually_exclusive :sha, :content_ref
+
         optional :dry_run, type: Boolean, default: false, desc: 'Run pipeline creation simulation, or only do static check. This is false by default'
         optional :include_jobs, type: Boolean, desc: 'If the list of jobs that would exist in a static check or pipeline
         simulation should be included in the response. This is false by default'
-        optional :ref, type: String, desc: 'Branch or tag used to execute a dry run. Defaults to the default branch of the project. Only used when dry_run is true'
+
+        optional :ref, type: String, desc: 'Deprecated: Use dry_run_ref instead'
+        optional :dry_run_ref, type: String, desc: 'Branch or tag used as context when executing a dry run. Defaults to the default branch of the project. Only used when dry_run is true'
+        mutually_exclusive :ref, :dry_run_ref
       end
 
       get ':id/ci/lint', urgency: :low do
         authorize_read_code!
 
-        sha = params[:sha] || user_project.repository.root_ref_sha
+        content_ref = params[:content_ref] || params[:sha] || user_project.repository.root_ref_sha
+        dry_run_ref = params[:dry_run_ref] || params[:ref] || user_project.default_branch
 
-        not_found! 'Commit' unless user_project.commit(sha).present?
+        commit = user_project.commit(content_ref)
+        not_found! 'Commit' unless commit.present?
 
-        content = user_project.repository.blob_data_at(sha, user_project.ci_config_path_or_default)
+        content = user_project.repository.blob_data_at(commit.sha, user_project.ci_config_path_or_default)
         result = Gitlab::Ci::Lint
-          .new(project: user_project, current_user: current_user, sha: sha)
-          .validate(content, dry_run: params[:dry_run], ref: params[:ref] || user_project.default_branch)
+          .new(project: user_project, current_user: current_user, sha: commit.sha)
+          .validate(content, dry_run: params[:dry_run], ref: dry_run_ref)
 
         present result, with: Entities::Ci::Lint::Result, current_user: current_user, include_jobs: params[:include_jobs]
       end

spec/requests/api/lint_spec.rb

@@ -4,10 +4,10 @@
 
 RSpec.describe API::Lint, feature_category: :pipeline_composition do
   describe 'GET /projects/:id/ci/lint' do
-    subject(:ci_lint) { get api("/projects/#{project.id}/ci/lint", api_user), params: { sha: sha, dry_run: dry_run, include_jobs: include_jobs } }
+    subject(:ci_lint) { get api("/projects/#{project.id}/ci/lint", api_user), params: { content_ref: content_ref, dry_run: dry_run, include_jobs: include_jobs } }
 
     let(:project) { create(:project, :repository) }
-    let(:sha) { nil }
+    let(:content_ref) { nil }
     let(:dry_run) { nil }
     let(:include_jobs) { nil }
 
@@ -323,6 +323,8 @@
             branch_name: 'master'
           )
 
+          project.repository.create_branch('invalid-content', 'master')
+
           project.repository.update_file(
             project.creator,
             '.gitlab-ci.yml',
@@ -330,11 +332,13 @@
             message: 'Automatically edited .gitlab-ci.yml again',
             branch_name: 'master'
           )
+
+          project.repository.create_branch('valid-content', 'master')
         end
 
         context 'when latest .gitlab-ci.yml is valid' do
-          # check with explicit sha
-          let(:sha) { project.repository.commit.sha }
+          # check with explicit content_ref
+          let(:content_ref) { project.repository.commit.sha }
 
           it 'passes validation' do
             ci_lint
@@ -349,7 +353,7 @@
         end
 
         context 'when previous .gitlab-ci.yml is invalid' do
-          let(:sha) { project.repository.commit.parent.sha }
+          let(:content_ref) { project.repository.commit.parent.sha }
 
           it 'fails validation' do
             ci_lint
@@ -359,12 +363,12 @@
             expect(json_response['merged_yaml']).to eq(first_edit)
             expect(json_response['valid']).to eq(false)
             expect(json_response['warnings']).to eq([])
-            expect(json_response['errors']).to eq(["jobs config should contain at least one visible job"])
+            expect(json_response['errors']).to eq(['jobs config should contain at least one visible job'])
           end
         end
 
         context 'when first .gitlab-ci.yml is valid' do
-          let(:sha) { project.repository.commit.parent.parent.sha }
+          let(:content_ref) { project.repository.commit.parent.parent.sha }
 
           it 'passes validation' do
             ci_lint
@@ -378,8 +382,8 @@
           end
         end
 
-        context 'when sha is not found' do
-          let(:sha) { "unknown" }
+        context 'when content_ref is not found' do
+          let(:content_ref) { 'unknown' }
 
           it 'returns 404 response' do
             ci_lint
@@ -387,6 +391,73 @@
             expect(response).to have_gitlab_http_status(:not_found)
           end
         end
+
+        context 'when sha (deprecated) is used with valid configuration' do
+          let(:sha) { project.repository.commit.sha }
+
+          it 'passes validation' do
+            get api("/projects/#{project.id}/ci/lint", api_user), params: { sha: sha, dry_run: dry_run, include_jobs: include_jobs }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response).to be_an Hash
+            expect(json_response['merged_yaml']).to eq(second_edit)
+            expect(json_response['valid']).to eq(true)
+            expect(json_response['warnings']).to eq([])
+            expect(json_response['errors']).to eq([])
+          end
+        end
+
+        context 'when sha (deprecated) and content_ref are used at the same time' do
+          let(:sha) { project.repository.commit.sha }
+
+          it 'returns bad request' do
+            get api("/projects/#{project.id}/ci/lint", api_user), params: { sha: sha, content_ref: sha }
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['error']).to eq('sha, content_ref are mutually exclusive')
+          end
+        end
+
+        context 'when ref (deprecated) and dry_run_ref are used at the same time' do
+          let(:sha) { project.repository.commit.sha }
+
+          it 'returns bad request' do
+            get api("/projects/#{project.id}/ci/lint", api_user), params: { ref: sha, dry_run_ref: sha }
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['error']).to eq('ref, dry_run_ref are mutually exclusive')
+          end
+        end
+
+        context 'when content_ref is a valid ref name with invalid config' do
+          let(:content_ref) { 'invalid-content' }
+
+          it 'fails validation' do
+            ci_lint
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response).to be_an Hash
+            expect(json_response['merged_yaml']).to eq(first_edit)
+            expect(json_response['valid']).to eq(false)
+            expect(json_response['warnings']).to eq([])
+            expect(json_response['errors']).to eq(["jobs config should contain at least one visible job"])
+          end
+        end
+
+        context 'when content_ref is a valid ref name with valid config' do
+          let(:content_ref) { 'valid-content' }
+
+          it 'passes validation' do
+            ci_lint
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response).to be_an Hash
+            expect(json_response['merged_yaml']).to eq(second_edit)
+            expect(json_response['valid']).to eq(true)
+            expect(json_response['warnings']).to eq([])
+            expect(json_response['errors']).to eq([])
+          end
+        end
       end
     end
   end