[rayci] support select & tag together as a filter (#234)

If both selects and tags are provided as a filter, select nodes based on both filtering. I need this for using select in microcheck, otherwise we will incur costs by saving all microcheck tests on PR. Let me know if there is a better alternative. Test: - CI --------- Signed-off-by: can <[email protected]>
ray-project · May 9, 2024 · ce9e1da · ce9e1da
1 parent e7eddf8
commit ce9e1da
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 34 deletions.
diff --git a/raycicmd/converter_test.go b/raycicmd/converter_test.go
@@ -540,7 +540,7 @@ func TestConvertPipelineGroup_priority(t *testing.T) {
 			{"commands": []string{"default priority"}},
 		},
 	}
-	filter := &stepFilter{tags: []string{}, runAll: true}
+	filter := &stepFilter{tags: []string{}, runAllTags: true}
 	bk, err := convertSingleGroup(c, g, filter)
 	if err != nil {
 		t.Fatalf("convert: %v", err)
@@ -589,7 +589,7 @@ func TestConvertPipelineGroup_dockerPlugin(t *testing.T) {
 			"mount_buildkite_agent": false,
 		}},
 	}
-	filter := &stepFilter{tags: []string{}, runAll: true}
+	filter := &stepFilter{tags: []string{}, runAllTags: true}
 	bk, err := convertSingleGroup(c, g, filter)
 	if err != nil {
 		t.Fatalf("convert: %v", err)

diff --git a/raycicmd/make.go b/raycicmd/make.go
@@ -90,15 +90,13 @@ func makePipeline(repoDir string, config *config, info *buildInfo) (
 
 	c := newConverter(config, info)
 
-	var filter *stepFilter
-	if len(info.selects) > 0 {
-		filter = newSelectStepFilter(config.SkipTags, info.selects)
-	} else {
-		f, err := newTagsStepFilter(config.SkipTags, config.TagFilterCommand)
-		if err != nil {
-			return nil, fmt.Errorf("run tag filter command: %w", err)
-		}
-		filter = f
+	filter, err := newStepFilter(
+		config.SkipTags,
+		info.selects,
+		config.TagFilterCommand,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("run tag filter command: %w", err)
 	}
 
 	// Build steps for CI.

diff --git a/raycicmd/step_filter.go b/raycicmd/step_filter.go
@@ -13,22 +13,28 @@ type stepFilter struct {
 	// selecting steps based on ID or key
 	selects map[string]bool
 
-	runAll bool
-	tags   []string
+	runAllTags bool
+	tags       []string
 }
 
 func (f *stepFilter) reject(step *stepNode) bool {
 	return step.hasTagIn(f.skipTags)
 }
 
 func (f *stepFilter) accept(step *stepNode) bool {
+	return f.acceptSelectHit(step) && f.acceptTagHit(step)
+}
+
+func (f *stepFilter) acceptSelectHit(step *stepNode) bool {
 	if f.selects != nil {
-		// in key selection mode, hit when the step has any of the keys.
 		return step.selectHit(f.selects)
 	}
 
-	// in tags filtering mode
-	if f.runAll {
+	return true
+}
+
+func (f *stepFilter) acceptTagHit(step *stepNode) bool {
+	if f.runAllTags {
 		return true
 	}
 
@@ -43,18 +49,24 @@ func (f *stepFilter) hit(step *stepNode) bool {
 	return !f.reject(step) && f.accept(step)
 }
 
-func newSelectStepFilter(skipTags []string, selects []string) *stepFilter {
-	filter := &stepFilter{skipTags: skipTags, selects: make(map[string]bool)}
-	for _, k := range selects {
-		filter.selects[k] = true
+func newStepFilter(
+	skipTags []string, selects []string, filterCmd []string,
+) (*stepFilter, error) {
+	filter, err := stepFilterFromCmd(skipTags, filterCmd)
+	if selects != nil && err == nil {
+		filter.selects = make(map[string]bool)
+		for _, k := range selects {
+			filter.selects[k] = true
+		}
 	}
-	return filter
+
+	return filter, err
 }
 
-func newTagsStepFilter(skips []string, filterCmd []string) (
+func stepFilterFromCmd(skips []string, filterCmd []string) (
 	*stepFilter, error,
 ) {
-	filter := &stepFilter{skipTags: skips, runAll: true}
+	filter := &stepFilter{skipTags: skips, runAllTags: true}
 
 	if len(filterCmd) == 0 {
 		return filter, nil
@@ -88,7 +100,7 @@ func newTagsStepFilter(skips []string, filterCmd []string) (
 	if len(tags) == 0 {
 		tags = nil
 	}
-	filter.runAll = false
+	filter.runAllTags = false
 	filter.tags = tags
 
 	return filter, nil

diff --git a/raycicmd/step_filter_test.go b/raycicmd/step_filter_test.go
@@ -23,24 +23,24 @@ func TestNewTagsStepFilter(t *testing.T) {
 		want: &stepFilter{},
 	}, {
 		cmd:  []string{},
-		want: &stepFilter{runAll: true},
+		want: &stepFilter{runAllTags: true},
 	}, {
 		cmd:  nil,
-		want: &stepFilter{runAll: true},
+		want: &stepFilter{runAllTags: true},
 	}, {
 		cmd:  []string{"echo", "*"},
-		want: &stepFilter{runAll: true},
+		want: &stepFilter{runAllTags: true},
 	}, {
 		skipTags: []string{"disabled"},
-		want:     &stepFilter{skipTags: []string{"disabled"}, runAll: true},
+		want:     &stepFilter{skipTags: []string{"disabled"}, runAllTags: true},
 	}, {
 		cmd:     []string{"exit", "1"},
 		wantErr: true,
 	}, {
 		cmd:  []string{"./local-not-exist.sh"},
-		want: &stepFilter{runAll: true},
+		want: &stepFilter{runAllTags: true},
 	}} {
-		got, err := newTagsStepFilter(test.skipTags, test.cmd)
+		got, err := newStepFilter(test.skipTags, nil, test.cmd)
 		if test.wantErr {
 			if err == nil {
 				t.Errorf("run %q: want error, got nil", test.cmd)
@@ -145,8 +145,8 @@ func TestStepFilter_tagsReject(t *testing.T) {
 
 func TestStepFilter_runAll(t *testing.T) {
 	filter := &stepFilter{
-		skipTags: []string{"disabled"},
-		runAll:   true,
+		skipTags:   []string{"disabled"},
+		runAllTags: true,
 	}
 
 	for _, tags := range [][]string{
@@ -173,7 +173,7 @@ func TestStepFilter_runAll(t *testing.T) {
 }
 
 func TestStepFilter_selects(t *testing.T) {
-	filter := newSelectStepFilter([]string{"disabled"}, []string{"foo", "bar"})
+	filter, _ := newStepFilter([]string{"disabled"}, []string{"foo", "bar"}, nil)
 	for _, node := range []*stepNode{
 		{key: "foo"},
 		{id: "foo"},
@@ -192,7 +192,7 @@ func TestStepFilter_selects(t *testing.T) {
 		}
 	}
 
-	filter = newSelectStepFilter([]string{"disabled"}, []string{"foo", "bar"})
+	filter, _ = newStepFilter([]string{"disabled"}, []string{"foo", "bar"}, nil)
 	for _, node := range []*stepNode{
 		{key: "f"},
 		{id: "f"},
@@ -204,3 +204,30 @@ func TestStepFilter_selects(t *testing.T) {
 		}
 	}
 }
+
+func TestStepFilter_selectsAndTags(t *testing.T) {
+	filter, _ := newStepFilter(
+		[]string{"disabled"},
+		[]string{"foo", "bar"},
+		[]string{"echo", "tune"},
+	)
+	for _, node := range []*stepNode{
+		{key: "foo"},
+		{id: "foo", tags: []string{"tune"}},
+		{id: "bar"},
+	} {
+		if !filter.accept(node) {
+			t.Errorf("miss %+v", node)
+		}
+	}
+
+	for _, node := range []*stepNode{
+		{id: "foo", tags: []string{"not_tune"}},
+		{id: "bar", tags: []string{"tune_not"}},
+		{key: "w00t"},
+	} {
+		if filter.accept(node) {
+			t.Errorf("miss %+v", node)
+		}
+	}
+}