-
Notifications
You must be signed in to change notification settings - Fork 41
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
adopt playground containers #200
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6b7db38
adopt playground containers
feloy 498b4a7
Update packages/backend/src/managers/playground.ts
feloy 458673e
fix unit tests
feloy ddfd4b3
use provider.onDidRegisterContainerConnection to know when providers …
feloy 1e1834c
start adopt when podman only provider starts
feloy 286cef4
create a dedicated class for listening the registration of podman pro…
feloy fdbb930
remove the getConnection method from PodmanConnection
feloy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/********************************************************************** | ||
* Copyright (C) 2024 Red Hat, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
***********************************************************************/ | ||
|
||
import { type RegisterContainerConnectionEvent, provider } from '@podman-desktop/api'; | ||
|
||
type startupHandle = () => void; | ||
|
||
export class PodmanConnection { | ||
#firstFound = false; | ||
#toExecuteAtStartup: startupHandle[] = []; | ||
|
||
init(): void { | ||
// In case the extension has not yet registered, we listen for new registrations | ||
// and retain the first started podman provider | ||
const disposable = provider.onDidRegisterContainerConnection((e: RegisterContainerConnectionEvent) => { | ||
if (e.connection.type !== 'podman' || e.connection.status() !== 'started') { | ||
return; | ||
} | ||
if (this.#firstFound) { | ||
return; | ||
} | ||
this.#firstFound = true; | ||
for (const f of this.#toExecuteAtStartup) { | ||
f(); | ||
} | ||
this.#toExecuteAtStartup = []; | ||
disposable.dispose(); | ||
}); | ||
|
||
// In case at least one extension has already registered, we get one started podman provider | ||
const engines = provider | ||
.getContainerConnections() | ||
.filter(connection => connection.connection.type === 'podman') | ||
.filter(connection => connection.connection.status() === 'started'); | ||
if (engines.length > 0) { | ||
disposable.dispose(); | ||
this.#firstFound = true; | ||
} | ||
} | ||
|
||
// startupSubscribe registers f to be executed when a podman container provider | ||
// registers, or immediately if already registered | ||
startupSubscribe(f: startupHandle): void { | ||
if (this.#firstFound) { | ||
f(); | ||
} else { | ||
this.#toExecuteAtStartup.push(f); | ||
} | ||
} | ||
} |
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we listening on connection registration andnot connection start/stop events ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this feature, I only want to list containers started before Podman Desktop is started, and I want to be informed when the podman extension is registered (or the
listContainers
would return an empty list if we try to retrieve the list before).(note that there is a
registries/ContainerRegistry
which is watching at events on a specific container, which we may merge with this class at some point)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test it so maybe i'm wrong but looking at the code it looks to me that it initializes the connection when the studio is activated if there is atleast one podman machine that is started and then it starts listening to if a machine gets registered.
So if i start my desktop having my podman machine stopped, the connection is not set as there is no podman machine running. If i start my podman machine in a second moment it does not trigger its initialization bc it only listen to machine registration.
So if i start the playground or any other feature that requires to retrieve the active connection it fails, no? But i have my podman machine running fine, i just started it after my desktop was activated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are probably right, I'll look at supporting this scenario too. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, I prefer for now to remove this exposure and use of
connection
from thePodmcnConnection
class, as it is not part of what is needed to fix the targeted issue, which is only related to getting a connection to the provider at startup.I'll work at this refactoring as part of another PR, if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a demo on Linux to see what you can test:
adopt-playground-2024-02-02_10.29.49.mp4
For now, we are not checking when a machine is stopped, to mark playground as stopped. It would be an extension of the PR #194 (and not particularly part of this PR as it is a different logic).
As said in my previous message, when you stop a playground container, it is also deleted (because marked as AUTOREMOVE). So you will never have a playground container (not even stopped) when you start a podman machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you stop the machine the container will be there, not deleted and you can restart it once you restart the machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, it is not the expected behaviour for a container marked as auto-removable. In Inspect of this container, is it marked as this?
I'm doing the same test in my Windows machine, and the container is not present anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I don't want to support these stopped containers, but if we consider having stopped playground containers, this adds a third state for the playground container in the UI (paused in addition to not-running and running), which we would also have to manage in the UI (set the playground in pause, etc). That would be a bigger change than just detecting a stopped container is started again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow response. Done it again now and i confirm that the Autoremove flag is there