Browse code

Minor commit validation fixes

* Update hack/cherry-pick.sh to produce valid commit summaries
* Improve validation examples
* Remove validation debugging output
* Ignore bump commits (for now)
* Allow unqualified non-kube repo references

Dan Mace authored on 2015/12/12 03:56:22
Showing 5 changed files
... ...
@@ -21,9 +21,19 @@ func main() {
21 21
 		os.Exit(1)
22 22
 	}
23 23
 
24
+	// TODO: Filter out bump commits for now until we decide how to deal with
25
+	// them correctly.
26
+	nonbumpCommits := []util.Commit{}
27
+	for _, commit := range commits {
28
+		if commit.DeclaresUpstreamChange() &&
29
+			!strings.HasPrefix(commit.Summary, "bump(") {
30
+			nonbumpCommits = append(nonbumpCommits, commit)
31
+		}
32
+	}
33
+
24 34
 	errs := []string{}
25 35
 	for _, validate := range AllValidators {
26
-		err := validate(commits)
36
+		err := validate(nonbumpCommits)
27 37
 		if err != nil {
28 38
 			errs = append(errs, err.Error())
29 39
 		}
... ...
@@ -4,6 +4,7 @@ import (
4 4
 	"bytes"
5 5
 	"fmt"
6 6
 	"regexp"
7
+	"strings"
7 8
 	"text/template"
8 9
 
9 10
 	"github.com/openshift/origin/cmd/rebasehelpers/util"
... ...
@@ -16,7 +17,7 @@ The following UPSTREAM commits have invalid summaries:
16 16
 {{ end }}
17 17
 UPSTREAM commit summaries should look like:
18 18
 
19
-  UPSTREAM: [non-kube-account/repo[/path]: ]<PR number>|'<carry>'|'<drop>': description
19
+  UPSTREAM: [non-kube-repo/name: ]<PR number|carry|drop>: description
20 20
 
21 21
 UPSTREAM commits which revert previous UPSTREAM commits should look like:
22 22
 
... ...
@@ -28,18 +29,19 @@ UPSTREAM commits are validated against the following regular expression:
28 28
 
29 29
 Examples of valid summaries:
30 30
 
31
-  UPSTREAM: 12345: An origin change
32
-  UPSTREAM: docker/docker: 12345: A docker change
31
+  UPSTREAM: 12345: A kube fix
32
+  UPSTREAM: coreos/etcd: 12345: An etcd fix
33 33
   UPSTREAM: <carry>: A carried kube change
34 34
   UPSTREAM: <drop>: A dropped kube change
35
-  UPSTREAM: revert: abcd123: docker/docker: 12345: A docker change
35
+  UPSTREAM: revert: abcd123: coreos/etcd: 12345: An etcd fix
36
+  UPSTREAM: k8s.io/heapster: 12345: A heapster fix
36 37
 
37 38
 `
38 39
 
39 40
 var AllValidators = []func([]util.Commit) error{
41
+	ValidateUpstreamCommitSummaries,
40 42
 	ValidateUpstreamCommitsWithoutGodepsChanges,
41 43
 	ValidateUpstreamCommitModifiesSingleGodepsRepo,
42
-	ValidateUpstreamCommitSummaries,
43 44
 	ValidateUpstreamCommitModifiesOnlyGodeps,
44 45
 	ValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo,
45 46
 }
... ...
@@ -54,7 +56,7 @@ func ValidateUpstreamCommitsWithoutGodepsChanges(commits []util.Commit) error {
54 54
 		}
55 55
 	}
56 56
 	if len(problemCommits) > 0 {
57
-		label := "The following commits contain Godeps changes but aren't declared as UPSTREAM:"
57
+		label := "The following commits contain Godeps changes but aren't declared as UPSTREAM"
58 58
 		msg := renderGodepFilesError(label, problemCommits, RenderOnlyGodepsFiles)
59 59
 		return fmt.Errorf(msg)
60 60
 	}
... ...
@@ -139,8 +141,7 @@ func ValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(commits []util.Commit)
139 139
 				return err
140 140
 			}
141 141
 			for _, changedRepo := range reposChanged {
142
-				if changedRepo != declaredRepo {
143
-					fmt.Printf("changedRepo=%s, declaredRepo=%s\n", changedRepo, declaredRepo)
142
+				if !strings.Contains(changedRepo, declaredRepo) {
144 143
 					problemCommits = append(problemCommits, commit)
145 144
 				}
146 145
 			}
... ...
@@ -208,14 +208,14 @@ func TestValidateUpstreamCommitSummaries(t *testing.T) {
208 208
 		{valid: true, summary: "UPSTREAM: k8s.io/heapster: 12345: a change"},
209 209
 		{valid: true, summary: "UPSTREAM: <carry>: a change"},
210 210
 		{valid: true, summary: "UPSTREAM: <drop>: a change"},
211
-		{valid: true, summary: "UPSTREAM: github.com/coreos/etcd: <carry>: a change"},
212
-		{valid: true, summary: "UPSTREAM: github.com/coreos/etcd: <drop>: a change"},
211
+		{valid: true, summary: "UPSTREAM: coreos/etcd: <carry>: a change"},
212
+		{valid: true, summary: "UPSTREAM: coreos/etcd: <drop>: a change"},
213 213
 		{valid: true, summary: "UPSTREAM: revert: abcd123: 12345: a change"},
214 214
 		{valid: true, summary: "UPSTREAM: revert: abcd123: k8s.io/heapster: 12345: a change"},
215 215
 		{valid: true, summary: "UPSTREAM: revert: abcd123: <carry>: a change"},
216 216
 		{valid: true, summary: "UPSTREAM: revert: abcd123: <drop>: a change"},
217
-		{valid: true, summary: "UPSTREAM: revert: abcd123: github.com/coreos/etcd: <carry>: a change"},
218
-		{valid: true, summary: "UPSTREAM: revert: abcd123: github.com/coreos/etcd: <drop>: a change"},
217
+		{valid: true, summary: "UPSTREAM: revert: abcd123: coreos/etcd: <carry>: a change"},
218
+		{valid: true, summary: "UPSTREAM: revert: abcd123: coreos/etcd: <drop>: a change"},
219 219
 		{valid: false, summary: "UPSTREAM: whoopsie daisy"},
220 220
 	}
221 221
 	for _, test := range tests {
... ...
@@ -251,7 +251,7 @@ func TestValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(t *testing.T) {
251 251
 				},
252 252
 				{
253 253
 					Sha:     "aaa0001",
254
-					Summary: "UPSTREAM: github.com/coreos/etcd: 12345: a change",
254
+					Summary: "UPSTREAM: coreos/etcd: 12345: a change",
255 255
 					Files: []util.File{
256 256
 						"Godeps/_workspace/src/k8s.io/kubernetes/file1",
257 257
 						"Godeps/_workspace/src/k8s.io/kubernetes/file2",
... ...
@@ -266,7 +266,7 @@ func TestValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(t *testing.T) {
266 266
 			commits: []util.Commit{
267 267
 				{
268 268
 					Sha:     "aaa0001",
269
-					Summary: "UPSTREAM: github.com/coreos/etcd: 12345: a change",
269
+					Summary: "UPSTREAM: coreos/etcd: 12345: a change",
270 270
 					Files: []util.File{
271 271
 						"Godeps/_workspace/src/github.com/coreos/etcd/file1",
272 272
 						"Godeps/_workspace/src/github.com/coreos/etcd/file2",
... ...
@@ -280,7 +280,7 @@ func TestValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(t *testing.T) {
280 280
 			commits: []util.Commit{
281 281
 				{
282 282
 					Sha:     "aaa0001",
283
-					Summary: "UPSTREAM: github.com/coreos/etcd: 12345: a change",
283
+					Summary: "UPSTREAM: coreos/etcd: 12345: a change",
284 284
 					Files: []util.File{
285 285
 						"Godeps/_workspace/src/github.com/coreos/etcd/a/file1",
286 286
 						"Godeps/_workspace/src/github.com/coreos/etcd/b/file2",
... ...
@@ -336,7 +336,7 @@ func TestValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(t *testing.T) {
336 336
 			commits: []util.Commit{
337 337
 				{
338 338
 					Sha:     "aaa0001",
339
-					Summary: "UPSTREAM: revert: abcd000: github.com/coreos/etcd: 12345: a change",
339
+					Summary: "UPSTREAM: revert: abcd000: coreos/etcd: 12345: a change",
340 340
 					Files: []util.File{
341 341
 						"Godeps/_workspace/src/k8s.io/kubernetes/file1",
342 342
 						"Godeps/_workspace/src/k8s.io/kubernetes/file2",
... ...
@@ -350,7 +350,7 @@ func TestValidateUpstreamCommitModifiesOnlyDeclaredGodepRepo(t *testing.T) {
350 350
 			commits: []util.Commit{
351 351
 				{
352 352
 					Sha:     "aaa0001",
353
-					Summary: "UPSTREAM: revert: abcd000: github.com/coreos/etcd: 12345: a change",
353
+					Summary: "UPSTREAM: revert: abcd000: coreos/etcd: 12345: a change",
354 354
 					Files: []util.File{
355 355
 						"Godeps/_workspace/src/github.com/coreos/etcd/file1",
356 356
 						"Godeps/_workspace/src/github.com/coreos/etcd/file2",
... ...
@@ -9,7 +9,7 @@ import (
9 9
 	"strings"
10 10
 )
11 11
 
12
-var UpstreamSummaryPattern = regexp.MustCompile(`UPSTREAM: (revert: [a-f0-9]{7,}: )?(([\w\.-]+\/[\w-]+)(\/[\w-]+)?: )?([0-9]{4,}:|<carry>:|<drop>:)`)
12
+var UpstreamSummaryPattern = regexp.MustCompile(`UPSTREAM: (revert: [a-f0-9]{7,}: )?(([\w\.-]+\/[\w-]+)?: )?(\d+:|<carry>:|<drop>:)`)
13 13
 
14 14
 // supportedHosts maps source hosts to the number of path segments that
15 15
 // represent the account/repo for that host. This is necessary because we
... ...
@@ -55,24 +55,6 @@ func (c Commit) DeclaredUpstreamRepo() (string, error) {
55 55
 	if len(repo) == 0 {
56 56
 		repo = "k8s.io/kubernetes"
57 57
 	}
58
-	// Do a simple special casing for repos which use 3 path segments to
59
-	// identify the repo. The only repos ever seen use either 2 or 3 so don't
60
-	// bother trying to make it too generic.
61
-	segments := -1
62
-	for host, count := range SupportedHosts {
63
-		if strings.HasPrefix(repo, host) {
64
-			segments = count
65
-			break
66
-		}
67
-	}
68
-	if segments == -1 {
69
-		fmt.Printf("commit modifies unsupported repo %q\n", repo)
70
-		return "", fmt.Errorf("commit modifies unsupported repo %q", repo)
71
-	}
72
-	fmt.Printf("repo=%s, segments=%d\n", repo, segments)
73
-	if segments == 3 {
74
-		repo += groups[4]
75
-	}
76 58
 	return repo, nil
77 59
 }
78 60
 
... ...
@@ -17,6 +17,7 @@ cd "${OS_ROOT}"
17 17
 repo="${UPSTREAM_REPO:-k8s.io/kubernetes}"
18 18
 package="${UPSTREAM_PACKAGE:-pkg/api}"
19 19
 UPSTREAM_REPO_LOCATION="${UPSTREAM_REPO_LOCATION:-../../../${repo}}"
20
+pr="$1"
20 21
 
21 22
 if [[ "$#" -ne 1 ]]; then
22 23
   echo "You must supply a pull request by number or a Git range in the upstream ${repo} project" 1>&2
... ...
@@ -40,7 +41,7 @@ os::build::require_clean_tree
40 40
 remote="${UPSTREAM_REMOTE:-origin}"
41 41
 git fetch ${remote}
42 42
 
43
-selector="$(os::build::commit_range $1 ${remote}/master)"
43
+selector="$(os::build::commit_range $pr ${remote}/master)"
44 44
 
45 45
 if [[ -z "${NO_REBASE-}" ]]; then
46 46
   echo "++ Generating patch for ${selector} onto ${lastrev} ..." 2>&1
... ...
@@ -80,9 +81,14 @@ if [[ $? -ne 0 ]]; then
80 80
   exit 1
81 81
 fi
82 82
 
83
+commit_message="UPSTREAM: $pr: Cherry-picked"
84
+if [ "$repo" != "k8s.io/kubernetes" ]; then
85
+  commit_message="UPSTREAM: $repo: $pr: Cherry-picked"
86
+fi
87
+
83 88
 set -o errexit
84 89
 git add .
85
-git commit -m "UPSTREAM: $1: " > /dev/null
90
+git commit -m "$commit_message" > /dev/null
86 91
 git commit --amend
87 92
 echo 2>&1
88 93
 echo "++ Done" 2>&1