Merge pull request #40 from semaphoreci/db/fix-change-in-for-non-mast…

…er-prs

Fix for non-master PRs

.semaphore/semaphore.yml

@@ -37,10 +37,12 @@ blocks:
           - git config --global user.name "Your Name"
           - unset SEMAPHORE_GIT_REF_TYPE
           - unset SEMAPHORE_GIT_BRANCH
+          - unset SEMAPHORE_GIT_PR_BRANCH
+          - unset SEMAPHORE_GIT_PR_SLUG
+          - unset SEMAPHORE_GIT_REPO_SLUG
           - unset SEMAPHORE_GIT_COMMIT_RANGE
           - unset SEMAPHORE_GIT_SHA
           - unset SEMAPHORE_MERGE_BASE
-          - unset SEMAPHORE_MERGE_BASE
 
       jobs:
         - name: go test
@@ -62,6 +64,7 @@ blocks:
                 - test/e2e/change_in_multiple_paths.rb
                 - test/e2e/change_in_on_forked_prs.rb
                 - test/e2e/change_in_on_prs.rb
+                - test/e2e/change_in_on_non_master_prs.rb
                 - test/e2e/change_in_on_tags.rb
                 - test/e2e/change_in_performance.rb
                 - test/e2e/change_in_pipeline_file_tracking.rb

lint.toml

@@ -41,7 +41,7 @@ warningCode = 1
 [rule.call-to-gc]
 
 [rule.cognitive-complexity]
-  arguments = [7]
+  arguments = [8]
 
 [rule.constant-logical-expr]
 [rule.context-as-argument]

pkg/cli/list_diff.go

@@ -27,11 +27,14 @@ var listDiffCmd = &cobra.Command{
 			return
 		}
 
-		fetchNeeded, fetchTarget := gitDiffSet.IsGitFetchNeeded()
+		fetchNeeded, fetchTargets := gitDiffSet.IsGitFetchNeeded()
+
 		if fetchNeeded {
-			output, err := git.Fetch(fetchTarget)
-			err = parseFetchError(fetchTarget, output, err)
-			check(err)
+			for _, fetchTarget := range fetchTargets {
+				output, err := git.Fetch(fetchTarget)
+				err = parseFetchError(fetchTarget, output, err)
+				check(err)
+			}
 		}
 
 		diffList, err := git.DiffList(gitDiffSet.CommitRange())

pkg/git/diff_set.go

@@ -67,21 +67,26 @@ func (d *DiffSet) IsEvaluationNeeded() bool {
 	return d.runningOnGitTag()
 }
 
-func (d *DiffSet) IsGitFetchNeeded() (bool, string) {
+func (d *DiffSet) IsGitFetchNeeded() (bool, []string) {
 	// We don't need to fetch any branch, we are evaluating the
 	// change in on the current branch.
 	if d.runningOnDefaultBranch() ||
 		d.runningOnForkedPullRequest() ||
 		d.isBaseCommitSha() {
-		return false, ""
+		return false, nil
 	}
 
+	fetchTargets := make([]string, 1)
+
 	commitRange := d.CommitRange()
+
+	fetchTargets[0] = commitRangeBase(commitRange)
+
 	if d.runningOnPullRequest() {
-		return true, commitRangeHead(commitRange)
-	} else {
-		return true, commitRangeBase(commitRange)
+		fetchTargets = append(fetchTargets, commitRangeHead(commitRange))
 	}
+
+	return true, fetchTargets
 }
 
 // commit range helpers

pkg/when/changein/function.go

@@ -26,13 +26,16 @@ func (f *Function) Eval() (bool, error) {
 		return f.GitDiffSet.OnTags, nil
 	}
 
-	fetchNeeded, fetchTarget := f.GitDiffSet.IsGitFetchNeeded()
+	fetchNeeded, fetchTargets := f.GitDiffSet.IsGitFetchNeeded()
+
 	if fetchNeeded {
-		output, err := git.Fetch(fetchTarget)
-		err = f.parseFetchError(fetchTarget, output, err)
+		for _, fetchTarget := range fetchTargets {
+			output, err := git.Fetch(fetchTarget)
+			err = f.parseFetchError(fetchTarget, output, err)
 
-		if err != nil {
-			return false, err
+			if err != nil {
+				return false, err
+			}
 		}
 	}
 

test/e2e/change_in_on_non_master_prs.rb

@@ -0,0 +1,116 @@
+# rubocop:disable all
+
+#
+# In pipelines triggered by PRs Semaphore checkouts the merge commit as a
+# detached head. In cases where PR is targeting non-master branch, there are
+# several tricky parts to test:
+# - actual cloned repo should only have master branch and a detached merged commit
+# - merge commit has changes on both the target and the PR branch,
+#   and we want only the PR ones
+# - we need to fetch both the target and the PR branch
+#
+# This test simulates this state by creating repo with two non-master branches,
+# merging them and reseting HEAD of target branch back by one so merge commit
+# becomes a detached head when it is checked out and then deleting the target
+# branch.
+#
+
+require_relative "../e2e"
+require 'yaml'
+
+pipeline = %{
+version: v1.0
+name: Test
+agent:
+  machine:
+    type: e1-standard-2
+
+blocks:
+  - name: Test
+    run:
+      when: "branch = 'master' and change_in('/app')"
+
+  - name: Test2
+    run:
+      when: "branch = 'master' and change_in('/test')"
+
+  - name: Test3
+    run:
+      when: "branch = 'master' and change_in('/lib')"
+}
+
+origin = TestRepoForChangeIn.setup()
+
+origin.add_file('.semaphore/semaphore.yml', pipeline)
+origin.commit!("Bootstrap")
+
+origin.create_branch("target-branch")
+origin.add_file("app/a.yml", "foo")
+origin.commit!("Bootstrap app")
+
+origin.create_branch("dev")
+origin.add_file("test/a.test", "bar")
+origin.commit!("Bootstrap tests")
+
+origin.switch_branch("target-branch")
+origin.add_file("lib/b.yml", "baz")
+origin.commit!("Bootstrap lib")
+
+origin.merge_branch("dev")
+
+# create a temp branch that will be fetched to get merge commit in local repo
+origin.create_branch("temp-branch-for-fetching-merge-commit")
+
+# delete merge commit from the target-branch
+origin.switch_branch("target-branch")
+origin.run(%{
+  git reset --hard HEAD~1
+})
+
+repo = origin.clone_local_copy(branch: "master")
+
+repo.run(%{
+  git fetch origin temp-branch-for-fetching-merge-commit
+
+  git branch temp-branch-for-fetching-merge-commit FETCH_HEAD
+
+  git checkout temp-branch-for-fetching-merge-commit
+
+  export SEMAPHORE_GIT_SHA=$(git rev-parse HEAD)
+
+  git reset --hard HEAD~1
+
+  git checkout $SEMAPHORE_GIT_SHA
+
+  git branch -D temp-branch-for-fetching-merge-commit
+
+  export SEMAPHORE_GIT_REF_TYPE=pull-request
+  export SEMAPHORE_GIT_BRANCH=target-branch
+  export SEMAPHORE_GIT_PR_BRANCH=dev
+
+  #{spc} compile \
+     --input .semaphore/semaphore.yml \
+     --output /tmp/output.yml \
+     --logs /tmp/logs.yml
+})
+
+assert_eq(YAML.load_file('/tmp/output.yml'), YAML.load(%{
+version: v1.0
+name: Test
+agent:
+  machine:
+    type: e1-standard-2
+
+blocks:
+  - name: Test
+    run:
+      when: "(branch = 'master') and false"
+
+  - name: Test2
+    run:
+      when: "(branch = 'master') and true"
+
+  - name: Test3
+    run:
+      when: "(branch = 'master') and false"
+}))