Skip to content

Add support for using regex patterns in findSuite #3456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
oharan2 wants to merge 2 commits into from
Closed

Add support for using regex patterns in findSuite #3456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90962d6
Add support for using regex patterns in findSuite
oharan2 Apr 21, 2026
1255211
Update pkg/db/suites.go
oharan2 Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions pkg/dataloader/prowloader/prow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,12 +1167,23 @@ func (pl *ProwLoader) findSuite(name string) *uint {
suite := &models.Suite{}
pl.dbc.DB.Where("name = ?", name).Find(&suite)
if suite.ID == 0 {
pl.suiteCache[name] = nil
} else {
id := suite.ID
pl.suiteCache[name] = &id
// No row - the exact suite name is not in the database (for example, by populateTestSuitesInDB)
if !db.IsTestSuiteImportable(name) {
Copy link
Copy Markdown
Contributor

@neisw neisw Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could IsTestSuiteImportable do the save of the suite? Claude noted the duplication of logic. Wondering if the save could be refactored within db/suites so that both populateTestSuitesInDB and IsTestSuiteImportable used the same method.

Duplicate pattern found in:

  • pkg/dataloader/prowloader/prow.go:1177-1181
  • pkg/db/suites.go:85-92 (in populateTestSuitesInDB)

Both locations contain identical suite creation logic:
suite.Name = name
tx := DB.Save(suite)
if tx.Error != nil {
log.WithError(...).Warningf(...)
return ...
}
log.WithField("suite", name).Info("created new test suite")

Recommendation: Extract into shared function:
// In pkg/db/suites.go
func createSuite(db *gorm.DB, name string) (*models.Suite, error)

pl.suiteCache[name] = nil
return nil
}
// If IsTestSuiteImportable() returns true (for example, pattern match), add the Suite row.
suite.Name = name
tx := pl.dbc.DB.Save(suite)
if tx.Error != nil {
log.WithError(tx.Error).Warningf("failed to create suite %q", name)
return nil
}
log.WithField("suite", name).Info("created new test suite")
}
return pl.suiteCache[name]
id := suite.ID
Copy link
Copy Markdown
Contributor

@neisw neisw Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious if ID would be set properly in the struct here since it is created during the save. The indication is that it should be. I wonder if it is still worth a check to verify it isn't 0 after the save though. Worst case would be that the first time it is save that job would be ignored if the ID was still 0. Next query it should exists.

● Analysis of suite.ID Population After Save()

You've identified a critical question! Let me trace through the code path:

suite := &models.Suite{} // ID = 0, Name = ""
pl.dbc.DB.Where("name = ?", name).Find(&suite) // Find fails, suite.ID still 0
if suite.ID == 0 {
suite.Name = name // ID = 0, Name = "lp-interop-foo"
tx := pl.dbc.DB.Save(suite) // INSERT operation
// ... error handling ...
}
id := suite.ID // ⚠️ Will this be populated?
pl.suiteCache[name] = &id
return &id

Expected GORM Behavior: YES, it should be populated

With GORM + PostgreSQL:

  1. Save() on a struct with ID == 0 triggers an INSERT
  2. PostgreSQL generates the ID via nextval()
  3. GORM uses the RETURNING id clause to retrieve it
  4. GORM populates suite.ID in the struct after the INSERT

pl.suiteCache[name] = &id
return &id
Comment on lines 1169 to +1186
Copy link
Copy Markdown

@etirta etirta Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a single array implementation, the solution is more straightforward (no call to an external function):

Suggested change
if suite.ID == 0 {
pl.suiteCache[name] = nil
} else {
id := suite.ID
pl.suiteCache[name] = &id
// No row - the exact suite name is not in the database (for example, by populateTestSuitesInDB)
if !db.IsTestSuiteImportable(name) {
pl.suiteCache[name] = nil
return nil
}
// If IsTestSuiteImportable() returns true (for example, pattern match), add the Suite row.
suite.Name = name
tx := pl.dbc.DB.Save(suite)
if tx.Error != nil {
log.WithError(tx.Error).Warningf("failed to create suite %q", name)
return nil
}
log.WithField("suite", name).Info("created new test suite")
}
return pl.suiteCache[name]
id := suite.ID
pl.suiteCache[name] = &id
return &id
if suite.ID == 0 {
pl.suiteCache[name] = nil
for _, re := range db.TestSuitePatterns {
if re.MatchString(name) {
suite.Name = name
result := pl.dbc.DB.Where("name = ?", name).FirstOrCreate(suite)
if result.Error == nil {
if result.RowsAffected > 0 {
log.WithField("suite", name).Info("created new test suite")
}
pl.suiteCache[name] = &suite.ID
} else {
log.WithError(result.Error).Warningf("failed to create suite %q", name)
}
break
}
}
} else {
pl.suiteCache[name] = &suite.ID
}
return pl.suiteCache[name]

}

func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJob, id uint, path string, junitPaths []string) ([]*models.ProwJobRunTest, int, sippyprocessingv1.JobOverallResult, error) {
Expand Down
40 changes: 40 additions & 0 deletions pkg/db/suites.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package db

import (
"regexp"

"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"gorm.io/gorm"
Expand Down Expand Up @@ -74,6 +76,44 @@ var testSuites = []string{
"tracing-uiplugin",
}

// testSuitePatterns are regular expressions (MatchString) for suite names that should be imported
// without listing every literal name. Patterns are compiled in init().
var testSuitePatterns = []string{
// LP interop naming: `lp-interop-<product>--<suffix>`.
`^lp-interop-`,
Comment on lines +82 to +83
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Regex pattern is broader than the documented suite format.

Line 82 documents lp-interop-<product>--<suffix>, but Line 83 (^lp-interop-) accepts malformed names (e.g., missing --<suffix>), which can import unintended suites.

Suggested tightening 
 var testSuitePatterns = []string{
 	// LP interop naming: `lp-interop-<product>--<suffix>`.
-	`^lp-interop-`,
+	`^lp-interop-.+--.+$`,
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// LP interop naming: `lp-interop-<product>--<suffix>`.
`^lp-interop-`,
var testSuitePatterns = []string{
// LP interop naming: `lp-interop-<product>--<suffix>`.
`^lp-interop-.+--.+$`,
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/db/suites.go` around lines 82 - 83, The current regex literal
`^lp-interop-` is too permissive and should be tightened to enforce the
documented format `lp-interop-<product>--<suffix>`; replace the pattern
`^lp-interop-` (in the regex list in pkg/db/suites.go) with a stricter pattern
such as `^lp-interop-[^-]+--.+$` (or a more restrictive charset like
`^lp-interop-[A-Za-z0-9]+--[A-Za-z0-9-]+$`) so names must include a product
segment and the required double-hyphen suffix.

Copy link
Copy Markdown
Contributor Author

@oharan2 oharan2 Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern lp-interop-<product>--<suffix> in the comment is an example

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oharan2, understood — the comment is illustrative, and the intentionally broad ^lp-interop- pattern is correct to cover all LP interop suite name variations without over-constraining them. My apologies for the noise.

📝 Learnings were identified and not saved (knowledge base disabled). Enable

}

var compiledTestSuitePatterns []*regexp.Regexp
Comment thread
oharan2 marked this conversation as resolved.

// Invalid regexes panic at process start.
func init() {
compiledTestSuitePatterns = make([]*regexp.Regexp, len(testSuitePatterns))
for i, p := range testSuitePatterns {
compiledTestSuitePatterns[i] = regexp.MustCompile(p)
}
}

// Runs inside the prow loader on every unknown suite name
// Reports whether junit from this testsuite name should be imported: either an
// exact entry in testSuites or a match against testSuitePatterns.
func IsTestSuiteImportable(name string) bool {
if name == "" {
return false
}
for _, s := range testSuites {
if s == name {
return true
}
}
for _, re := range compiledTestSuitePatterns {
if re.MatchString(name) {
return true
}
}
return false
}

// Runs when the DB is set up / migrated.
func populateTestSuitesInDB(db *gorm.DB) error {
for _, suiteName := range testSuites {
s := models.Suite{}
Expand Down
28 changes: 28 additions & 0 deletions pkg/db/suites_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package db

import "testing"

func TestIsTestSuiteImportable(t *testing.T) {
tests := []struct {
name string
want bool
}{
{"", false},
{"openshift-tests", true},
{"CNV-lp-interop", true},
{"ACS-lp-interop", true},
{"lp-interop-ACS--my-tests", true},
{"lp-interop-Foo", true},
{"CNV-lp-interop-extra-suffix", false},
{"random-suite", false},
{"-lp-interop", false},
{"lp-interop", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsTestSuiteImportable(tt.name); got != tt.want {
t.Errorf("IsTestSuiteImportable(%q) = %v, want %v", tt.name, got, tt.want)
}
})
}
}