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.

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

EVG-20943: Ensure virtual workstation is Linux architecture #7088

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

sophstad
Copy link
Contributor

@sophstad sophstad commented Sep 27, 2023

EVG-20943

Description

  • Implement this line as backend validation: ensure that distros that can be used as virtual workstations use Linux architecture

Testing

  • Update unit tests

Documentation

N/A

@sophstad sophstad requested review from a team September 28, 2023 14:48
@sophstad sophstad marked this pull request as ready for review September 28, 2023 14:48

if !utility.StringSliceContains(linuxDistros, d.Arch) {
errs = append(errs, ValidationError{
Message: "spawnable distros must use Linux architecture",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this say workstation distros? some windows distros are user spawnable on prod

Copy link
Contributor

@Kimchelly Kimchelly left a comment

Choose a reason for hiding this comment

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

LGTM mod minor suggestions.


if !utility.StringSliceContains(linuxDistros, d.Arch) {
errs = append(errs, ValidationError{
Message: "workstation distros must use Linux architecture",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The phrasing is a little weird because Linux isn't an architecture. The arch enum represents a combination of OS + CPU architecture, so phrasing this as "workstation distros must use Linux with a supported CPU architecture" (or something like that) would sound less ambiguous.

@@ -547,6 +547,20 @@ func ensureHasValidVirtualWorkstationSettings(ctx context.Context, d *distro.Dis
Level: Error,
})
}
linuxDistros := []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe allowedArchs (arches...?) or linuxArchs since distros aren't relevant here.

@sophstad sophstad merged commit 4fbee12 into evergreen-ci:main Sep 29, 2023
9 checks passed
@sophstad sophstad deleted the EVG-20943 branch September 29, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants