Adds Orbstack Container Runtime Engine #1761
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schnie
force-pushed
the
feature/orbstack-engine
branch
from
f15adca to
9149d07
Compare
schnie
commented
Dec 12, 2024
Comment on lines
+9
to
+18
| func (d orbstackEngine) Start() (string, error) { | ||
| openDockerCmd := Command{ | ||
| Command: open, | ||
| Args: []string{ | ||
| "-a", | ||
| orbstack, | ||
| }, | ||
| } | ||
| return openDockerCmd.Execute() | ||
| } |
There was a problem hiding this comment.
Here's the custom logic for orbstack. We just ensure the engine is up and running before we proceed.
schnie
commented
Dec 12, 2024
airflow/runtimes/docker_runtime.go
Outdated
Comment on lines
30
to
71
| // CreateDockerRuntimeWithDefaults creates a new DockerRuntime with the default DockerEngine and OSChecker. | ||
| func CreateDockerRuntimeWithDefaults() DockerRuntime { | ||
| return DockerRuntime{Engine: GetDockerEngine(CreateHostInspectorWithDefaults()), OSChecker: CreateOSChecker()} | ||
| } | ||
|
|
||
| // GetDockerEngine returns the appropriate DockerEngine based on the binary discovered. | ||
| func GetDockerEngine(inspector HostInspector) DockerEngine { | ||
| engine, err := GetDockerEngineBinary(inspector) | ||
| if err != nil { | ||
| return new(dockerEngine) | ||
| } | ||
|
|
||
| // Return the appropriate container runtime based on the binary discovered. | ||
| switch engine { | ||
| case orbctl: | ||
| return new(orbstackEngine) | ||
| default: | ||
| return new(dockerEngine) | ||
| } | ||
| } | ||
|
|
||
| // GetDockerEngineBinary returns the first Docker binary found in the $PATH environment variable. | ||
| // This is used to determine which Engine to use. Eg: orbctl or docker. | ||
| func GetDockerEngineBinary(inspector HostInspector) (string, error) { | ||
| // If the orbctl binary is found, it means orbstack is installed. The docker binary would also exist | ||
| // in this case, but we use this as a signal to use the orbstack engine. We'll still end up | ||
| // shelling our docker commands out using the docker binary. | ||
| binaries := []string{orbctl, docker} | ||
|
|
||
| // Get the $PATH environment variable. | ||
| pathEnv := inspector.GetEnvVar("PATH") | ||
| for _, binary := range binaries { | ||
| if found := FindBinary(pathEnv, binary, inspector); found { | ||
| return binary, nil | ||
| } | ||
| } | ||
|
|
||
| // If no binary is found, we just return our default docker binary. | ||
| // The higher level check for the container runtime binary will handle the user-facing error. | ||
| return docker, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
This is new logic associated with constructing a new default implementation of the DockerRuntime. Instead of just using the DockerEngine, we check the machine for orbctl to see if we should run the custom Start() method.
schnie
commented
Dec 12, 2024
Comment on lines
+14
to
+24
| // mockFileChecker is a mock implementation of FileChecker for tests in this file. | ||
| type mockFileChecker struct { | ||
| ExistingFiles map[string]bool | ||
| } | ||
|
|
||
| // FileExists is just a mock for os.Stat(). In our test implementation, we just check | ||
| // if the file exists in the list of mocked files for a given test. | ||
| func (m mockFileChecker) FileExists(path string) bool { | ||
| return m.ExistingFiles[path] | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved this special mock here to live with its specific tests. I've also configured mockery to now generate a more standard mock under mocks for the other tests.
schnie
commented
Dec 12, 2024
Comment on lines
-51
to
+55
| return CreateDockerRuntime(new(dockerEngine), new(osChecker)), nil | ||
| return CreateDockerRuntimeWithDefaults(), nil | ||
| case podman: | ||
| return CreatePodmanRuntime(new(podmanEngine), new(osChecker)), nil | ||
| return CreatePodmanRuntimeWithDefaults(), nil |
There was a problem hiding this comment.
Instead of specifying these concrete implementations out here, we've added new constructor functions to construct with the default concrete implementations. This lets us encapsulate the new logic to determine the specific Engine to plug in.
schnie
force-pushed
the
feature/orbstack-engine
branch
from
9149d07 to
96a4103
Compare
schnie
commented
Dec 12, 2024
Comment on lines
+1
to
+30
| package runtimes | ||
|
|
||
| type HostInterrogator interface { | ||
| IsMac() bool | ||
| IsWindows() bool | ||
| FileExists(string) bool | ||
| GetEnvVar(string) string | ||
| } | ||
|
|
||
| type hostInterrogator struct { | ||
| OSChecker | ||
| FileChecker | ||
| EnvChecker | ||
| } | ||
|
|
||
| func CreateHostInspector(osChecker OSChecker, fileChecker FileChecker, envChecker EnvChecker) HostInterrogator { | ||
| return hostInterrogator{ | ||
| osChecker, | ||
| fileChecker, | ||
| envChecker, | ||
| } | ||
| } | ||
|
|
||
| func CreateHostInspectorWithDefaults() HostInterrogator { | ||
| return hostInterrogator{ | ||
| CreateOSChecker(), | ||
| CreateFileChecker(), | ||
| CreateEnvChecker(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of passing all 3 of these implementations around, I've consolidated them into a HostInterrogator object that can answer all the questions about the host. In our tests we mock out anything that interacts with the underlying host.
schnie
requested review from
neel-astro,
jeremybeard and
kushalmalani
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds an Orbstack Container Runtime Engine. This specific engine plugs into the existing Docker Runtime, as they both follow the same Runtime pattern. The Orbstack Engine embeds the original Docker Engine and takes advantage of its
IsRunning()function. It implements its own customStart()method to runopen -a orbstackinstead of docker.Like the Docker Desktop implemenation we've had support for a while, this only works on Mac at the moment. Windows and Linux users will get a message to start the container runtime manually and try again.
If Orbstack is running in the background, nothing additional happens, we just start the containers. If it's not running, the Desktop app starts and the engine loads in the background. The window can be closed out just like Docker Desktop while the engine runs in the background.
🎟 Issue(s)
Related to https://github.com/astronomer/astro/issues/24344
🧪 Functional Testing
Performed all the local dev commands locally with multiple container runtimes to verify no regressions.
Added and refactored tests.
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft