Repository: jenkins-x/lighthouse
Pull request #1072 (2020-09-21 04:38:44)

 fix: merging postsubmits & presubmits together by name to avoid duplicates
We merge presubmits or postsubmits by name when using shared configurations and in-repo configurations; or when using in-repo configurations on a particular branch versus master.

When merging triggers we need to make sure we combine them together if they have the same name; otherwise we end up with a configuration validation error with duplicate presubmits/postsubmits by name or for the same branch on a post submit

Without this fix, its quite easy to get lighthouse to fail to process a webhook if it has in-repo configurations as it ends up creating a duplicate Presubmit/Postsubmit and then refuses to process the webhook


jenkins-x-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **jstrachan**
You can assign the PR to them by writing `/assign @jstrachan` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=jenkins-x%2Flighthouse).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/jenkins-x/lighthouse/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["jstrachan"]} -->
jenkins-x-bot: @jstrachan: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pr-build | 4fa42253ccc4832419069435c05e149dc9af31dd | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072/1) | `/test pr-build`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jstrachan: /retest
jenkins-x-bot: @jstrachan: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pr-build | 4ec47e1fca2751f96b7551d3c0a508f105457a56 | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072/9) | `/test pr-build`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jstrachan: /retest
abayer: I'm not entirely sure what this is doing - we can have multiple postsubmits as long as they have different names.
jenkins-x-bot: @jstrachan: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pr-build | 5ed351e9943785cd5a30fcd21637c7ceebd7f1d5 | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072/15) | `/test pr-build`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1072) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jstrachan: @abayer sorry for the cryptic comments/PR. 

I've tried updating the above comment to be a bit more clear. Basically the logic when merging either shared + in repo pre/post submits; or master + custom branch in repo pre/post submits we were not merging by name but just appending everything together into a list; so it was very easy to create validation errors if the pre/post submits for a repository had duplicates for the same name/branch.

This change should not have any effect on folks not using in-repo configuration at all; but should help avoid LH failing unnecessarily with "duplicate pre/post submit errors"
jstrachan: incidentally I've squashed the commits + also modified the postsubmit logic to be polymorphic with presubmits; the merge is always based on names. So shared + in repo (or master + custom branch in repo) merging always combines pre/post submits together if they are the same name; otherwise they are added to the list
jstrachan: /retest
abayer: /lgtm
jenkins-x-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *[abayer](https://github.com/jenkins-x/lighthouse/pull/1072#issuecomment-697344811 "LGTM")*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=jenkins-x%2Flighthouse).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com#issuecomment-697344811/jenkins-x/lighthouse/blob/master/OWNERS)~~ [abayer]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/triggerconfig/merge/merge.go b/pkg/triggerconfig/merge/merge.go
index fa2ae9f6..34a0c81a 100644
--- a/pkg/triggerconfig/merge/merge.go
+++ b/pkg/triggerconfig/merge/merge.go
@@ -17,8 +17,17 @@ func ConfigMerge(cfg *config.Config, pluginsCfg *plugins.Configuration, repoConf
 		}
 
 		ps := cfg.Presubmits[repoKey]
-		for _, p := range repoConfig.Spec.Presubmits {
-			ps = append(ps, p)
+		for i, p := range repoConfig.Spec.Presubmits {
+			found := false
+			for _, pt2 := range ps {
+				if pt2.Name == p.Name {
+					ps[i] = p
+					found = true
+				}
+			}
+			if !found {
+				ps = append(ps, p)
+			}
 		}
 		cfg.Presubmits[repoKey] = ps
 	}
@@ -28,8 +37,17 @@ func ConfigMerge(cfg *config.Config, pluginsCfg *plugins.Configuration, repoConf
 		}
 
 		ps := cfg.Postsubmits[repoKey]
-		for _, p := range repoConfig.Spec.Postsubmits {
-			ps = append(ps, p)
+		for i, p := range repoConfig.Spec.Postsubmits {
+			found := false
+			for _, pt2 := range ps {
+				if pt2.Name == p.Name {
+					ps[i] = p
+					found = true
+				}
+			}
+			if !found {
+				ps = append(ps, p)
+			}
 		}
 		cfg.Postsubmits[repoKey] = ps
 	}


Test patch:
diff --git a/pkg/triggerconfig/inrepo/calculate_test.go b/pkg/triggerconfig/inrepo/calculate_test.go
index 1c53aad4..e52528b8 100644
--- a/pkg/triggerconfig/inrepo/calculate_test.go
+++ b/pkg/triggerconfig/inrepo/calculate_test.go
@@ -110,6 +110,6 @@ func TestTriggersInBranchMergeToMaster(t *testing.T) {
 	assert.Contains(t, presubmitNames, "lint", "presubmits for repo %s", fullName)
 	assert.Contains(t, presubmitNames, "newthingy", "presubmits for repo %s", fullName)
 
-	assert.Len(t, cfg.Postsubmits[fullName], 0, "postsubmits for repo %s", fullName)
+	assert.Len(t, cfg.Postsubmits[fullName], 1, "postsubmits for repo %s", fullName)
 
 }
diff --git a/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/master/.lighthouse/triggers/triggers.yaml b/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/master/.lighthouse/triggers/triggers.yaml
index 372fea95..6930052a 100644
--- a/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/master/.lighthouse/triggers/triggers.yaml
+++ b/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/master/.lighthouse/triggers/triggers.yaml
@@ -9,4 +9,7 @@ spec:
     trigger: "/lint"
     rerunCommand: "/relint"
     agent: tekton-pipeline
-
+  postsubmits:
+  - name: release
+    context: "release"
+    agent: tekton-pipeline
diff --git a/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/mybranch/.lighthouse/triggers/triggers.yaml b/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/mybranch/.lighthouse/triggers/triggers.yaml
index aabcc87b..6167aef2 100644
--- a/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/mybranch/.lighthouse/triggers/triggers.yaml
+++ b/pkg/triggerconfig/inrepo/test_data/myorg/branchtest/refs/mybranch/.lighthouse/triggers/triggers.yaml
@@ -2,6 +2,13 @@ apiVersion: config.lighthouse.jenkins-x.io/v1alpha1
 kind: TriggerConfig
 spec:
   presubmits:
+  - name: lint
+    context: "lint"
+    alwaysRun: true
+    optional: false
+    trigger: "/lint"
+    rerunCommand: "/relint"
+    agent: tekton-pipeline
   - name: newthingy
     context: "newthingy"
     alwaysRun: true
@@ -9,4 +16,7 @@ spec:
     trigger: "/newthingy"
     rerunCommand: "/renewthingy"
     agent: tekton-pipeline
-
+  postsubmits:
+  - name: release
+    context: "release"
+    agent: tekton-pipeline