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

fix: purge #1456

Open
wants to merge 3 commits into
base: target-workspace-refactor
Choose a base branch
from
Open

fix: purge #1456

wants to merge 3 commits into from

Conversation

lbrecic
Copy link
Contributor

@lbrecic lbrecic commented Dec 12, 2024

Purge revision

Description

This PR revisions purge functionality in regards to changes introduced with refactor.

  • Targets, workspaces and builds are now not automatically purged, but user is rather prompted whether or not they wish to continue the purge with existing resources or if they would rather remove them manually before the complete purge thus making purge removing only CLI dependent files such as configuration files, ssh files, etc.

  • All AwaitEmptyList methods are completely removed.

  • Server.Purge method is removed since it became obsolete once automated resource purge is removed.

  • Runner purging providers is implemented via daytona runner purge command implementation.

  • Local runner providers purge is implemented within the daytona purge command.

  • A health-check endpoint is added to Daytona runner in order to execute daytona runner purge correctly.

  • This change requires a documentation update

  • I have made corresponding changes to the documentation

@lbrecic lbrecic added the Discussion Needs consensus label Dec 12, 2024
@Tpuljak Tpuljak force-pushed the target-workspace-refactor branch from 6780931 to b67a139 Compare December 23, 2024 10:14
@lbrecic lbrecic force-pushed the purge-revision branch 2 times, most recently from b6bdf4c to 84a5d97 Compare December 23, 2024 16:59
@daytonaBot
Copy link
Collaborator

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 14 days). It will be closed if no further activity occurs. Thank you for your contribution.

@Tpuljak Tpuljak added never stale An issue that never goes stale and removed Stale labels Jan 7, 2025
@lbrecic lbrecic force-pushed the purge-revision branch 3 times, most recently from 93c9a44 to affdf9c Compare January 9, 2025 14:48
@lbrecic lbrecic removed the Discussion Needs consensus label Jan 9, 2025
Signed-off-by: Luka Brecic <[email protected]>
@lbrecic lbrecic marked this pull request as ready for review January 9, 2025 16:26
@lbrecic lbrecic requested review from a team as code owners January 9, 2025 16:26
return err
}

var serverStoppedCheck bool
Copy link
Member

Choose a reason for hiding this comment

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

Should be called runnerStoppedCheck

Copy link
Member

Choose a reason for hiding this comment

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

Also, you need to run the health check before the prompt. There's no need for the prompt if the runner is not running.

)

func PurgeResourcesPrompt(continuePurge *bool, numOfTargets, numOfWorkspaces, numOfBuilds int) {
titleMsg := fmt.Sprintf("Leftover resources found: [targets: %d, workspaces: %d, builds: %d]\nWould you like to continue with purge?", numOfTargets, numOfWorkspaces, numOfBuilds)
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a bit more user friendly.

  1. Only display resources that are len > 0
  2. Display the suggested commands to run to remove those resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2025-01-10.at.14.06.08.mov

pkg/cmd/server/serve.go Outdated Show resolved Hide resolved
pkg/cmd/purge.go Outdated Show resolved Hide resolved
Signed-off-by: Luka Brecic <[email protected]>
@lbrecic lbrecic requested a review from Tpuljak January 10, 2025 13:07
}
}
if !continuePurge {
fmt.Printf("\nOperation cancelled.\nManually delete leftover resources for a complete purge by running following commands:\n%s\n", strings.Join(commands, "\n"))
Copy link
Member

Choose a reason for hiding this comment

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

Add one more new line after by running following commands:.

Also, I suggest that we remove Delete {targets|workspaces|builds: and just print out the commands. That's easier to copy paste.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
never stale An issue that never goes stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants