Skip to content

Conversation

@mirkoCrobu
Copy link
Contributor

Motivation

closes #105

We could add a new app destroycommand (or a similar name) for stopping and removing all the data of an app.
In particular, we should do a docker-compose down and remove the .cache.

We should also consider adding a destroyed (or a better name) that is the default state of an app if it has not been run yet.
With this additional state, we would then be able to always return a state when listing the apps (right now apps and examples that have not been run, will show up as with no state).

Change description

Additional Notes

Reviewer checklist

  • PR addresses a single concern.
  • PR title and description are properly filled.
  • Changes will be merged in main.
  • Changes are covered by tests.
  • Logging is meaningful in case of troubleshooting.

@mirkoCrobu mirkoCrobu self-assigned this Dec 9, 2025
@mirkoCrobu
Copy link
Contributor Author

Add uninitialized status and set status information for app list

image

@mirkoCrobu mirkoCrobu requested review from a team and lucarin91 December 10, 2025 15:28
@mirkoCrobu mirkoCrobu marked this pull request as ready for review December 15, 2025 08:22
Copy link
Contributor

@lucarin91 lucarin91 left a comment

Choose a reason for hiding this comment

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

We should also add an endpoint for doing this (maybe in a subsequent PR)

@mirkoCrobu
Copy link
Contributor Author

We should also add an endpoint for doing this (maybe in a subsequent PR)

[API] Add a destroy app command

@mirkoCrobu mirkoCrobu requested a review from a team December 17, 2025 14:58
@mirkoCrobu mirkoCrobu force-pushed the issue_105_add_app_destroy_command branch from 274a600 to ef16820 Compare December 22, 2025 10:48
@mirkoCrobu mirkoCrobu requested a review from lucarin91 December 22, 2025 11:09
Comment on lines +153 to +156
return &AppStatusInfo{
AppPath: paths.New(pathLabel),
Status: StatusUninitialized,
}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this is a valid app? Maybe we still need to return nil nil here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to have a valid app without containers. But, to be sure we also need to perform an app validation within the filesystem or something like that. So for now, it is better to keep the nil nil and maybe handle this use case in a different PR if we decide it is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I guess it would be better to move the whole part about introducing a new 'uninitialized' app status(valid app with no containers) to a dedicated PR, and focus this one just on implementing the destroy command.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I would keep it, maybe we can add this check to be sure we always return the status of an existing app

diff --git a/internal/orchestrator/helpers.go b/internal/orchestrator/helpers.go
index 3c1564e..b762418 100644
--- a/internal/orchestrator/helpers.go
+++ b/internal/orchestrator/helpers.go
@@ -150,10 +150,14 @@ func getAppStatusByPath(
                return nil, fmt.Errorf("failed to list containers: %w", err)
        }
        if len(containers) == 0 {
-               return &AppStatusInfo{
-                       AppPath: paths.New(pathLabel),
-                       Status:  StatusUninitialized,
-               }, nil
+               path := paths.New(pathLabel)
+               if _, err := app.Load(path); err == nil {
+                       return &AppStatusInfo{
+                               AppPath: path,
+                               Status:  StatusUninitialized,
+                       }, nil
+               }
+               return nil, nil
        }

        app := parseAppStatus(containers)

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.

Add a destroy app command

2 participants