size: composite configuration (#79)

In retrospect, the previous config for size was not very fortunate as it
splits properties that have a semantic relation in two unrelated ones:

    - label: M
        size-above: 10
        size-below: 100

In order to support file exclusions for the size calculation it'd make a
cleaner config schema to have all size properties in a single Size node.

This change introduces a `size` node as a condition configuration that
replaces `size-above` and `size-below`, while respecting backwards
compatibility. Older config files will continue working, but from now on
the way to specify the file condition will be:

        - label: M
            above: 10
            below: 100

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>

* fixup! size: composite configuration

---------

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>

README.md

@@ -348,12 +348,15 @@ For example, given this `.github/labeler.yml`:
 
 ```yaml
 - label: "S"
-  size-below: 10
+  size:
+      below: 10
 - label: "M"
-  size-above: 9
-  size-below: 100
+  size:
+      above: 9
+      below: 100
 - label: "L"
-  size-above: 100
+  size:
+      above: 100
 ```
 
 These would be the labels assigned to some PRs, based on their size as
@@ -365,6 +368,11 @@ reported by the [GitHub API](https://developer.github.com/v3/pulls).
 |Second example|5|42|M|
 |Third example|68|148|L|
 
+**NOTICE** the old format for specifying size properties (`size-above`
+and `size-below`) has been deprecated. The action will continue
+supporting old configs for now, but users are encouraged to migrate to
+the new configuration schema.
+
 ### Title <a name="title" />
 
 This condition is satisfied when the title matches on the given regex.

cmd/action.go

@@ -34,7 +34,7 @@ func main() {
 		return
 	}
 
-	config, err := getLabelerConfig(configRaw)
+	config, err := getLabelerConfigV1(configRaw)
 	if err != nil {
 		return
 	}
@@ -89,23 +89,23 @@ func getRepoFile(gh *github.Client, repo, file, sha string) (*[]byte, error) {
 	return &raw, err
 }
 
-// getLabelerConfig builds a LabelerConfigV1 from a raw yaml
-func getLabelerConfig(configRaw *[]byte) (*labeler.LabelerConfigV1, error) {
+// getLabelerConfigV1 builds a LabelerConfigV1 from a raw yaml
+func getLabelerConfigV1(configRaw *[]byte) (*labeler.LabelerConfigV1, error) {
 	var c labeler.LabelerConfigV1
 	err := yaml.Unmarshal(*configRaw, &c)
 	if err != nil {
 		log.Printf("Unable to unmarshall config %s: ", err)
 	}
 	if c.Version == 0 {
-		c, err = getLabelerConfigV1(configRaw)
+		c, err = getLabelerConfigV0(configRaw)
 		if err != nil {
 			log.Printf("Unable to unmarshall legacy config %s: ", err)
 		}
 	}
 	return &c, err
 }
 
-func getLabelerConfigV1(configRaw *[]byte) (labeler.LabelerConfigV1, error) {
+func getLabelerConfigV0(configRaw *[]byte) (labeler.LabelerConfigV1, error) {
 
 	// Load v0
 	var oldCfg map[string]labeler.LabelMatcher

cmd/action_test.go

@@ -3,10 +3,12 @@ package main
 import (
 	"io/ioutil"
 	"os"
+	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	l "github.com/srvaroa/labeler/pkg"
+	labeler "github.com/srvaroa/labeler/pkg"
 )
 
 func TestGetLabelerConfigV0(t *testing.T) {
@@ -22,7 +24,7 @@ func TestGetLabelerConfigV0(t *testing.T) {
 	}
 
 	var c *l.LabelerConfigV1
-	c, err = getLabelerConfig(&contents)
+	c, err = getLabelerConfigV1(&contents)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -85,7 +87,7 @@ func TestGetLabelerConfigV1(t *testing.T) {
 	}
 
 	var c *l.LabelerConfigV1
-	c, err = getLabelerConfig(&contents)
+	c, err = getLabelerConfigV1(&contents)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -165,7 +167,7 @@ func TestGetLabelerConfigV1WithIssues(t *testing.T) {
 	}
 
 	var c *l.LabelerConfigV1
-	c, err = getLabelerConfig(&contents)
+	c, err = getLabelerConfigV1(&contents)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -189,6 +191,47 @@ func TestGetLabelerConfigV1WithIssues(t *testing.T) {
 	}
 }
 
+func TestGetLabelerConfigV1WithCompositeSize(t *testing.T) {
+
+	file, err := os.Open("../test_data/config_v1_composite_size.yml")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	contents, err := ioutil.ReadAll(file)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var c *l.LabelerConfigV1
+	c, err = getLabelerConfigV1(&contents)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expect := l.LabelerConfigV1{
+		Version: 1,
+		Labels: []l.LabelMatcher{
+			{
+				Label:     "S",
+				SizeAbove: "1",
+				SizeBelow: "10",
+			},
+			{
+				Label: "M",
+				Size: &labeler.SizeConfig{
+					Above: "9",
+					Below: "100",
+				},
+			},
+		},
+	}
+
+	if !reflect.DeepEqual(expect, *c) {
+		t.Fatalf("\nExpect: %#v \nGot: %#v", expect, *c)
+	}
+}
+
 func TestGetLabelerConfig2V1(t *testing.T) {
 
 	file, err := os.Open("../test_data/config2_v1.yml")
@@ -202,7 +245,7 @@ func TestGetLabelerConfig2V1(t *testing.T) {
 	}
 
 	var c *l.LabelerConfigV1
-	c, err = getLabelerConfig(&contents)
+	c, err = getLabelerConfigV1(&contents)
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/condition_size.go

@@ -16,15 +16,34 @@ func SizeCondition() Condition {
 			return target.ghPR != nil
 		},
 		Evaluate: func(target *Target, matcher LabelMatcher) (bool, error) {
-			if len(matcher.SizeBelow) == 0 && len(matcher.SizeAbove) == 0 {
-				return false, fmt.Errorf("size-above and size-below are not set in config")
+
+			if isNewConfig(matcher) && isOldConfig(matcher) {
+				log.Printf("WARNING: you are using both the old " +
+					"`size-above`/`size-below` settings together with " +
+					"the newer `size`. You should use only the latter. " +
+					"This condition will apply the configurations set in `Size` " +
+					"and ignore the rest")
+			}
+
+			realMatcher := matcher.Size
+			if realMatcher == nil {
+				if matcher.SizeBelow == "" && matcher.SizeAbove == "" {
+					return false, fmt.Errorf("no size conditions are set in config")
+				}
+				realMatcher = &SizeConfig{
+					Above: matcher.SizeAbove,
+					Below: matcher.SizeBelow,
+				}
 			}
-			upperBound, err := strconv.ParseInt(matcher.SizeBelow, 0, 64)
+
+			log.Printf("Checking PR size using config: %+v", realMatcher)
+
+			upperBound, err := strconv.ParseInt(realMatcher.Below, 0, 64)
 			if err != nil {
 				upperBound = math.MaxInt64
 				log.Printf("Upper boundary set to %d (config has invalid or empty value)", upperBound)
 			}
-			lowerBound, err := strconv.ParseInt(matcher.SizeAbove, 0, 32)
+			lowerBound, err := strconv.ParseInt(realMatcher.Above, 0, 32)
 			if err != nil || lowerBound < 0 {
 				lowerBound = 0
 				log.Printf("Lower boundary set to 0 (config has invalid or empty value)")
@@ -36,3 +55,11 @@ func SizeCondition() Condition {
 		},
 	}
 }
+
+func isNewConfig(matcher LabelMatcher) bool {
+	return matcher.Size != nil
+}
+
+func isOldConfig(matcher LabelMatcher) bool {
+	return matcher.SizeAbove != "" || matcher.SizeBelow != ""
+}

pkg/labeler.go

@@ -8,6 +8,11 @@ import (
 	gh "github.com/google/go-github/v50/github"
 )
 
+type SizeConfig struct {
+	Above string
+	Below string
+}
+
 type LabelMatcher struct {
 	AuthorCanMerge string `yaml:"author-can-merge"`
 	Authors        []string
@@ -19,9 +24,16 @@ type LabelMatcher struct {
 	Label          string
 	Mergeable      string
 	Negate         bool
-	SizeAbove      string `yaml:"size-above"`
-	SizeBelow      string `yaml:"size-below"`
-	Title          string
+	Size           *SizeConfig
+	// size-legacy
+	// These two are unused in the codebase (they get copied inside
+	// the Size object), but we keep them to respect backwards
+	// compatiblity parsing older configs without adding more
+	// complexity.
+	SizeAbove string `yaml:"size-above"`
+	SizeBelow string `yaml:"size-below"`
+	// size-legacy
+	Title string
 }
 
 type LabelerConfigV0 map[string]LabelMatcher

pkg/labeler_test.go

@@ -258,6 +258,25 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"M"},
 		},
+		{
+			event:    "pull_request",
+			payloads: []string{"mid_pr"},
+			name:     "Test the size_below and size_above rules with new composite config",
+			config: LabelerConfigV1{
+				Version: 1,
+				Labels: []LabelMatcher{
+					{
+						Label: "M",
+						Size: &SizeConfig{
+							Above: "9",
+							Below: "100",
+						},
+					},
+				},
+			},
+			initialLabels:  []string{},
+			expectedLabels: []string{"M"},
+		},
 		{
 			event:    "pull_request",
 			payloads: []string{"big_pr"},

test_data/config_v1_composite_size.yml

@@ -0,0 +1,9 @@
+version: 1
+labels:
+  - label: S
+    size-below: 10
+    size-above: 1
+  - label: M
+    size:
+      above: 9
+      below: 100