Mac PM test: Require pre-installed rosetta #22773
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
pkg/machine/e2e/machine_test.go
Outdated
| if testProvider.VMType() == define.AppleHvVirt { | ||
| cmd := exec.Command("softwareupdate", "--install-rosetta", "--agree-to-license") | ||
| cmd := exec.Command("arch", "-arch", "x86_64", "/usr/bin/uname", "-m") | ||
| err := cmd.Run() | ||
| if err != nil { | ||
| Fail(fmt.Sprintf("Command failed with error: %q", err)) | ||
| Fail(fmt.Sprintf("Rosetta doesn't appear to be installed or functional, 'uname' test command failed with error: %q", err)) | ||
| } |
There was a problem hiding this comment.
I think we should move this into the cirrus setup scripts and not the test suite itself
There was a problem hiding this comment.
I'm concerned if somebody ever wants to run the tests locally, outside of CI, then they'll simply fail. These failures may be very obscure (I have yet to test this) as the actual binary could be executing inside a VM or inside a container inside a VM (I'm really not sure how these tests work).
There was a problem hiding this comment.
I'm not opposed to adding the check in both places, since failing slightly earlier has a chance of speeding development activities.
There was a problem hiding this comment.
This check only makes sense for the rosetta tests, I don't think it should fail all other tests.
There was a problem hiding this comment.
Please correct me if I'm wrong, but don't the rosetta and non-rosetta tests all run as one ginkgo "batch"?
Either way, I'm really not a golang or code base expert, so don't feel comfortable making more precise/targeted changes. Maybe this part could be done as a followup PR?
There was a problem hiding this comment.
Just like any sane test framework you can run tests individually which I do locally.
I really don't see the point of such check, it is not like the tests validate other dependencies either and if rosetta test fails it is likely easy enough to figure
There was a problem hiding this comment.
Okay, point taken. I'll make a half-hearted attempt to remove the install code, but no guarantees. It may need to be done as a followup.
Edit: By my read, it seems like I can simply remove the installation code from the test.
cevich
force-pushed
the
use_preinstalled_rosetta
branch
3 times, most recently
from
c557d42 to
29cccf0
Compare
cevich
changed the title
cevich
force-pushed
the
use_preinstalled_rosetta
branch
2 times, most recently
from
679bd13 to
670e93a
Compare
You've spotted it exactly, I was just about to add a comment why this PR is still marked as draft 😀 Edit/note to me: All workers should have been recycled w/ necessary (new) setup by 2024-05-23T11:00:00 UTC |
cevich
changed the title
Previously, the mac podman-machine tests installed rosetta before executing any tests. As a best-practice (and because the Macs in CI are shared) tests should never permanently modify the system. As of this commit, the system setup script used for the CI Macs does the rosetta installation. Remove the test setup code that installed rosetta and add a CI-level confirmation that it's been pre-installed. Signed-off-by: Chris Evich <[email protected]>
cevich
force-pushed
the
use_preinstalled_rosetta
branch
from
670e93a to
74e8f98
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@mheon @edsantiago PTAL when you get a chance, this should be ready to go. |
|
/lgtm |
|
Looks like this needs an approval also. |
|
/approval |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cevich, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/cherrypick v5.1 |
|
@cevich: new pull request created: #22889 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Previously, the mac podman-machine tests installed rosetta before
executing any tests. As a best-practice (and because the Macs in CI are
shared) tests should never permanently modify the system. As of this
commit, the system setup script used for the CI Macs does the rosetta
installation. Remove the test setup code that installed rosetta and
add a CI-level confirmation that it's been pre-installed.
Ref:
TODO:
Does this PR introduce a user-facing change?