Skip to content
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

DEVPROD-6656 surface distro warnings in validator #7913

Merged
merged 3 commits into from
May 24, 2024
Merged

DEVPROD-6656 surface distro warnings in validator #7913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2d7c51
DEVPROD-6656 surface distro warnings in validator
ablack12 May 22, 2024
f91e50d
remove todo
ablack12 May 22, 2024
f32f5db
cr
ablack12 May 24, 2024
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
2 changes: 1 addition & 1 deletion validator/distro_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func CheckDistro(ctx context.Context, d *distro.Distro, s *evergreen.Settings, n
var allDistroIDs, allDistroAliases []string
var err error
if newDistro || len(d.Aliases) > 0 {
allDistroIDs, allDistroAliases, err = getDistros(ctx)
allDistroIDs, allDistroAliases, _, err = getDistros(ctx)
if err != nil {
return nil, err
}
Expand Down
69 changes: 49 additions & 20 deletions validator/project_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,39 +195,47 @@ func ValidationErrorsToString(ves ValidationErrors) string {
}

// getDistros creates a slice of all distro IDs and aliases.
func getDistros(ctx context.Context) (ids []string, aliases []string, err error) {
func getDistros(ctx context.Context) (ids []string, aliases []string, distroWarnings map[string]string, err error) {
return getDistrosForProject(ctx, "")
}

// getDistrosForProject creates a slice of all valid distro IDs and a slice of
// all valid aliases for a project. If projectID is empty, it returns all distro
// all valid aliases for a project, as well as any distro warnings. If projectID is empty, it returns all distro
// IDs and all aliases.
func getDistrosForProject(ctx context.Context, projectID string) (ids []string, aliases []string, err error) {
func getDistrosForProject(ctx context.Context, projectID string) (ids []string, aliases []string, distroWarnings map[string]string, err error) {
distroWarnings = map[string]string{}
// create a slice of all known distros
distros, err := distro.AllDistros(ctx)
if err != nil {
return nil, nil, err
return nil, nil, nil, err
}
for _, d := range distros {
if projectID != "" && len(d.ValidProjects) > 0 {
if utility.StringSliceContains(d.ValidProjects, projectID) {
ids = append(ids, d.Id)
for _, alias := range d.Aliases {
if !utility.StringSliceContains(aliases, alias) {
aliases = append(aliases, alias)
}
}
}
} else {
if projectID == "" || len(d.ValidProjects) == 0 || utility.StringSliceContains(d.ValidProjects, projectID) {
ids = append(ids, d.Id)
for _, alias := range d.Aliases {
if !utility.StringSliceContains(aliases, alias) {
aliases = append(aliases, alias)
}
}
if d.WarningNote != "" {
addDistroWarning(distroWarnings, d.Id, d.WarningNote)
distroWarnings[d.Id] = d.WarningNote
for _, alias := range d.Aliases {
addDistroWarning(distroWarnings, alias, d.WarningNote)

}
}
}
}
return ids, aliases, nil
return ids, utility.UniqueStrings(aliases), distroWarnings, nil
}

func addDistroWarning(distroWarnings map[string]string, distroName, warningNote string) {
if distroWarnings[distroName] == "" {
distroWarnings[distroName] = warningNote
return
}
distroWarnings[distroName] = fmt.Sprintf("\t%s\n\t%s", distroWarnings[distroName], warningNote)
}

// CheckProject calls the validating logic for a Project's configuration.
Expand Down Expand Up @@ -303,7 +311,7 @@ func CheckProjectErrors(ctx context.Context, project *model.Project, includeLong
}

// get distro IDs and aliases for ensureReferentialIntegrity validation
distroIDs, distroAliases, err := getDistrosForProject(ctx, project.Identifier)
distroIDs, distroAliases, distroWarnings, err := getDistrosForProject(ctx, project.Identifier)
if err != nil {
validationErrs = append(validationErrs, ValidationError{Message: "can't get distros from database"})
}
Expand All @@ -314,7 +322,7 @@ func CheckProjectErrors(ctx context.Context, project *model.Project, includeLong
}
containerNameMap[container.Name] = true
}
validationErrs = append(validationErrs, ensureReferentialIntegrity(project, containerNameMap, distroIDs, distroAliases)...)
validationErrs = append(validationErrs, ensureReferentialIntegrity(project, containerNameMap, distroIDs, distroAliases, distroWarnings)...)
return validationErrs
}

Expand All @@ -333,7 +341,7 @@ func CheckPatchedProjectConfigErrors(patchedProjectConfig string) ValidationErro
return CheckProjectConfigErrors(projectConfig)
}

// verify that the project configuration syntax is valid
// CheckProjectConfigErrors verifies that the project configuration syntax is valid
func CheckProjectConfigErrors(projectConfig *model.ProjectConfig) ValidationErrors {
validationErrs := ValidationErrors{}
if projectConfig == nil {
Expand All @@ -343,6 +351,7 @@ func CheckProjectConfigErrors(projectConfig *model.ProjectConfig) ValidationErro
validationErrs = append(validationErrs,
projectConfigErrorValidator(projectConfig)...)
}

return validationErrs
}

Expand Down Expand Up @@ -859,8 +868,8 @@ func validateBuildVariantTaskNames(task string, variant string, allTaskNames map
return errs
}

// ensureReferentialIntegrity checks all fields that reference other entities defined in the YAML and ensure that they are referring to valid names.
func ensureReferentialIntegrity(project *model.Project, containerNameMap map[string]bool, distroIDs []string, distroAliases []string) ValidationErrors {
// ensureReferentialIntegrity checks all fields that reference other entities defined in the YAML and ensure that they are referring to valid names, and returns any relevant distro warnings.
func ensureReferentialIntegrity(project *model.Project, containerNameMap map[string]bool, distroIDs []string, distroAliases []string, distroWarnings map[string]string) ValidationErrors {
errs := ValidationErrors{}
// create a set of all the task names
allTaskNames := map[string]bool{}
Expand Down Expand Up @@ -915,6 +924,16 @@ func ensureReferentialIntegrity(project *model.Project, containerNameMap map[str
},
)
}
if warning, ok := distroWarnings[name]; ok {
errs = append(errs,
ValidationError{
Message: fmt.Sprintf("task '%s' in buildvariant '%s' "+
"references distro '%s' with the following admin-defined warning(s): %s",
task.Name, buildVariant.Name, name, warning),
Level: Warning,
Copy link
Contributor

@Kimchelly Kimchelly May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just for my understanding, these are not actual warnings from the distro validator, correct? They're just warnings that distro admins can manually set to give notice to users about some kind of issue?

Should we give some kind of heads up that users might see distro warnings or maybe that evergreen validate could give emit distro-related warnings? I know some teams use evergreen validate to check the validity of their YAMLs, but if they get this kind of warning, there's not much they can do about it because the warning comes from the distro, not their YAML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Admins haven't actually started using this yet so we don't need to give a heads up yet anyway, but I think that's a good point. I can tweak this wording, and surface with the infra team that they may want to communicate if they're going to be using frequently (and to be careful about usage -- like they should probably only use this if the distro is being removed soon or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could address the warning by switching distros, which I think is the point.

},
)
}
if utility.StringSliceContains(distroIDs, name) {
runOnHasDistro = true
}
Expand Down Expand Up @@ -946,6 +965,16 @@ func ensureReferentialIntegrity(project *model.Project, containerNameMap map[str
},
)
}
if warning, ok := distroWarnings[name]; ok {
errs = append(errs,
ValidationError{
Message: fmt.Sprintf("buildvariant '%s' "+
"references distro '%s' with the following admin-defined warning: %s",
buildVariant.Name, name, warning),
Level: Warning,
},
)
}
if utility.StringSliceContains(distroIDs, name) {
runOnHasDistro = true
}
Expand Down
108 changes: 81 additions & 27 deletions validator/project_validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2527,6 +2527,10 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
Convey("When validating a project", t, func() {
distroIds := []string{"rhel55"}
distroAliases := []string{"rhel55-alias"}
distroWarnings := map[string]string{
"rhel55": "55 is not the best number",
"rhel55-alias": "and this is not the best alias",
}
Convey("an error should be thrown if a referenced task for a "+
"buildvariant does not exist", func() {
project := &model.Project{
Expand All @@ -2544,11 +2548,10 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
},
},
}
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases)
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases, nil)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 1)
})

Convey("no error should be thrown if a referenced task for a "+
"buildvariant does exist", func() {
project := &model.Project{
Expand All @@ -2559,15 +2562,52 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
{
Name: "linux",
Tasks: []model.BuildVariantTaskUnit{
{Name: "compile", Variant: "linux"},
{Name: "compile", Variant: "linux", RunOn: []string{"rhel55"}},
},
},
},
}
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases), ShouldResemble,
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases, nil), ShouldResemble,
ValidationErrors{})
})
Convey("an error should be thrown if a task references a distro has a warning", func() {
project := &model.Project{
Tasks: []model.ProjectTask{
{Name: "compile"},
},
BuildVariants: []model.BuildVariant{
{
Name: "linux",
Tasks: []model.BuildVariantTaskUnit{
{Name: "compile", Variant: "linux", RunOn: []string{"rhel55"}},
},
},
},
}
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases, distroWarnings)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 1)
})
Convey("an error should be thrown if a variant references a distro has a warning", func() {
project := &model.Project{
Tasks: []model.ProjectTask{
{Name: "compile"},
},
BuildVariants: []model.BuildVariant{
{
Name: "linux",
RunOn: []string{"rhel55-alias"},
Tasks: []model.BuildVariantTaskUnit{
{Name: "compile", Variant: "linux"},
},
},
},
}
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases, distroWarnings)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 1)

})
Convey("an error should be thrown if a referenced distro for a "+
"buildvariant does not exist", func() {
project := &model.Project{
Expand All @@ -2578,9 +2618,8 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
},
},
}
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases)
So(errs, ShouldNotResemble,
ValidationErrors{})
errs := ensureReferentialIntegrity(project, nil, distroIds, distroAliases, nil)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 1)
})

Expand All @@ -2597,9 +2636,8 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
containerNameMap := map[string]bool{
"rhel55": true,
}
errs := ensureReferentialIntegrity(project, containerNameMap, distroIds, distroAliases)
So(errs, ShouldNotResemble,
ValidationErrors{})
errs := ensureReferentialIntegrity(project, containerNameMap, distroIds, distroAliases, nil)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 2)
So(errs[0].Message, ShouldContainSubstring, "buildvariant 'enterprise' references a container name overlapping with an existing distro 'rhel55'")
So(errs[1].Message, ShouldContainSubstring, "run_on cannot contain a mixture of containers and distros")
Expand All @@ -2617,9 +2655,8 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
containerNameMap := map[string]bool{
"c1": true,
}
errs := ensureReferentialIntegrity(project, containerNameMap, distroIds, distroAliases)
So(errs, ShouldNotResemble,
ValidationErrors{})
errs := ensureReferentialIntegrity(project, containerNameMap, distroIds, distroAliases, nil)
So(errs, ShouldNotResemble, ValidationErrors{})
So(len(errs), ShouldEqual, 1)
So(errs[0].Message, ShouldContainSubstring, "run_on cannot contain a mixture of containers and distros")
})
Expand All @@ -2634,7 +2671,7 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
},
},
}
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases), ShouldResemble, ValidationErrors{})
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases, nil), ShouldResemble, ValidationErrors{})
})

Convey("no error should be thrown if a referenced distro alias for a"+
Expand All @@ -2647,7 +2684,7 @@ func TestEnsureReferentialIntegrity(t *testing.T) {
},
},
}
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases), ShouldResemble, ValidationErrors{})
So(ensureReferentialIntegrity(project, nil, distroIds, distroAliases, nil), ShouldResemble, ValidationErrors{})
})
})
}
Expand Down Expand Up @@ -4136,42 +4173,59 @@ func TestGetDistrosForProject(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
require.NoError(db.Clear(distro.Collection))
d := distro.Distro{
d1 := distro.Distro{
Id: "distro1",
Aliases: []string{"distro1-alias", "distro1and2-alias"},
ValidProjects: []string{"project1", "project2"},
WarningNote: "this is a warning for the first distro",
}
require.NoError(d.Insert(ctx))
d = distro.Distro{
Id: "distro2",
Aliases: []string{"distro2-alias", "distro1and2-alias"},
require.NoError(d1.Insert(ctx))
d2 := distro.Distro{
Id: "distro2",
Aliases: []string{"distro2-alias", "distro1and2-alias"},
WarningNote: "this is the warning for another distro",
}
require.NoError(d.Insert(ctx))
d = distro.Distro{
require.NoError(d2.Insert(ctx))
d3 := distro.Distro{
Id: "distro3",
ValidProjects: []string{"project5"},
}
require.NoError(d.Insert(ctx))
require.NoError(d3.Insert(ctx))

ids, aliases, err := getDistros(ctx)
ids, aliases, warnings, err := getDistros(ctx)
require.NoError(err)
require.Len(ids, 3)
require.Len(aliases, 3)
require.Len(warnings, 5)
assert.Contains(aliases, "distro1and2-alias")
assert.Contains(aliases, "distro1-alias")
assert.Contains(aliases, "distro2-alias")
ids, aliases, err = getDistrosForProject(ctx, "project1")
assert.Equal(warnings[d1.Id], d1.WarningNote)
assert.Equal(warnings[d2.Id], d2.WarningNote)
assert.Equal(warnings["distro1-alias"], d1.WarningNote)
assert.Equal(warnings["distro2-alias"], d2.WarningNote)
assert.Contains(warnings["distro1and2-alias"], d1.WarningNote)
assert.Contains(warnings["distro1and2-alias"], d2.WarningNote)

ids, aliases, warnings, err = getDistrosForProject(ctx, "project1")
require.NoError(err)
require.Len(ids, 2)
require.Len(warnings, 5) // Both d1 and d2 are going to match here
assert.Contains(ids, "distro1")
assert.Contains(aliases, "distro1and2-alias")
assert.Contains(aliases, "distro1-alias")
ids, aliases, err = getDistrosForProject(ctx, "project3")

// Only d2 is going to match here
ids, aliases, warnings, err = getDistrosForProject(ctx, "project3")
require.NoError(err)
require.Len(ids, 1)
assert.Len(warnings, 3)
assert.Contains(ids, "distro2")
assert.Contains(aliases, "distro2-alias")
assert.Contains(aliases, "distro1and2-alias")
assert.Equal(warnings[d2.Id], d2.WarningNote)
assert.Equal(warnings["distro2-alias"], d2.WarningNote)
assert.Equal(warnings["distro1and2-alias"], d2.WarningNote)
}

func TestValidateTaskSyncCommands(t *testing.T) {
Expand Down Expand Up @@ -5614,7 +5668,7 @@ func TestValidateTaskGroupsInBV(t *testing.T) {
}
for testName, testCase := range tests {
t.Run(testName, func(t *testing.T) {
errs := ensureReferentialIntegrity(&testCase.project, nil, []string{}, []string{})
errs := ensureReferentialIntegrity(&testCase.project, nil, []string{}, []string{}, nil)
if testCase.expectErr {
assert.Equal(t, errs[0].Message, testCase.expectedErrMsg)
} else {
Expand Down
Loading