toolshed/abra
4
4
Fork 23
Code Issues 82 Pull Requests 12 Packages Releases 45 Activity

feat: backup revolution #361

Merged
decentral1se merged 7 commits from backup-revolution into main 2024-01-12 20:52:34 +00:00
Conversation 9 Commits 7 Files Changed 10
+401 -559
decentral1se commented 2023-10-01 06:24:12 +00:00
Owner
Copy Link
Copy Source

See coop-cloud/organising#485.

Building on work in coop-cloud/backup-bot-two#23.

Agreed on in R011: https://docs.coopcloud.tech/federation/resolutions/passed/011

See https://git.coopcloud.tech/coop-cloud/organising/issues/485. Building on work in https://git.coopcloud.tech/coop-cloud/backup-bot-two/pulls/23. Agreed on in R011: https://docs.coopcloud.tech/federation/resolutions/passed/011
decentral1se force-pushed backup-revolution from cf37076865 to ed0ab7fb90 2023-10-04 20:52:23 +00:00 Compare
decentral1se force-pushed backup-revolution from ed0ab7fb90 to c4c73d2ba8 2023-10-10 06:43:41 +00:00 Compare
decentral1se force-pushed backup-revolution from c4c73d2ba8 to 17513967de 2023-10-10 06:50:22 +00:00 Compare
decentral1se force-pushed backup-revolution from 17513967de to 02a83057fc 2023-10-11 07:21:27 +00:00 Compare
moritz requested changes 2023-10-11 13:12:20 +00:00
moritz left a comment
Member
Copy Link
Copy Source

Thank you for the work! So existing, we're getting very close to abra backupbot integration!

Thank you for the work! So existing, we're getting very close to `abra` backupbot integration!
🎉 1
cli/app/backup.go Outdated
@@ -414,0 +392,4 @@
}
if targetPath != "" {
logrus.Debugf("including TARGET=%s in backupbot exec invocation", targetPath)
execEnv = append(execEnv, fmt.Sprintf("TARGET=%s", targetPath))
moritz commented 2023-10-11 13:09:52 +00:00
Member
Copy Link
Copy Source

The TARGET env need to be part of restore. For listing snapshots it's not relevant.

The `TARGET` env need to be part of restore. For listing snapshots it's not relevant.
decentral1se commented 2023-10-11 15:12:47 +00:00
Author
Owner
Copy Link
Copy Source

Ah yes, nice one. Removed and added to restore logic. Force pushed but unsure if it's showing up yet. Thanks for the review!

Ah yes, nice one. Removed and added to `restore` logic. Force pushed but unsure if it's showing up yet. Thanks for the review!
decentral1se marked this conversation as resolved
decentral1se force-pushed backup-revolution from 02a83057fc to 5127966438 2023-10-11 14:52:28 +00:00 Compare
decentral1se commented 2023-10-11 15:13:16 +00:00
Author
Owner
Copy Link
Copy Source

I'm gonna try put out a point release before merging this.

Commences integration test suite engines...

I'm gonna try put out a point release before merging this. *Commences integration test suite engines...*
moritz reviewed 2023-10-12 10:52:33 +00:00
cli/app/restore.go Outdated
@@ -135,0 +98,4 @@
AttachStderr: true,
AttachStdin: true,
AttachStdout: true,
Cmd: []string{"/usr/bin/backup", "--", "snapshots"},
moritz commented 2023-10-12 10:52:33 +00:00
Member
Copy Link
Copy Source

snapshots need to be replaced with restore

`snapshots` need to be replaced with `restore`
👍 1
decentral1se marked this conversation as resolved
decentral1se force-pushed backup-revolution from 5127966438 to 63387a011a 2023-10-12 13:09:21 +00:00 Compare
decentral1se force-pushed backup-revolution from 63387a011a to 7ee89d3a8f 2024-01-09 10:47:10 +00:00 Compare
decentral1se commented 2024-01-09 11:25:42 +00:00
Author
Owner
Copy Link
Copy Source

Trying to finish the last step of this to get it off my plate...

Final steps for abra app backup download ... implementation...

Trying to finish the last step of this to get it off my plate... Final steps for `abra app backup download ...` implementation...
decentral1se force-pushed backup-revolution from 7ee89d3a8f to 529cb9bd12 2024-01-11 14:38:26 +00:00 Compare
decentral1se changed title from WIP: backup revolution to feat: backup revolution 2024-01-11 14:39:00 +00:00
decentral1se commented 2024-01-11 14:40:11 +00:00
Author
Owner
Copy Link
Copy Source

Ready For Review 🎉

Ready For Review 🎉
decentral1se requested review from moritz 2024-01-11 15:04:07 +00:00
decentral1se requested review from p4u1 2024-01-11 15:04:16 +00:00
p4u1 reviewed 2024-01-11 15:38:42 +00:00
cli/app/backup.go
@@ -135,3 +83,1 @@
if !ok {
logrus.Fatalf("no backup config for %s? does %s exist?", serviceName, serviceName)
}
chosenService, err := service.GetServiceByLabel(context.Background(), cl, config.BackupbotLabel, internal.NoInput)
p4u1 commented 2024-01-11 15:37:27 +00:00
Owner
Copy Link
Copy Source

Could add a helper function to retrieve the backup bot container

Could add a helper function to retrieve the backup bot container
p4u1 marked this conversation as resolved
cli/app/backup.go
@@ -153,3 +112,1 @@
logrus.Fatal(err)
}
}
execBackupListOpts := types.ExecConfig{
p4u1 commented 2024-01-11 15:31:20 +00:00
Owner
Copy Link
Copy Source

Maybe add a helper functions to exec the backup script on the remote container?

Maybe add a helper functions to exec the backup script on the remote container?
p4u1 marked this conversation as resolved
cli/app/restore.go Outdated
@@ -40,4 +38,3 @@
},
Before: internal.SubCommandBefore,
BashComplete: autocomplete.AppNameComplete,
Description: `
p4u1 commented 2024-01-11 15:21:04 +00:00
Owner
Copy Link
Copy Source

Why remove the description? :)

Why remove the description? :)
decentral1se commented 2024-01-12 15:51:06 +00:00
Author
Owner
Copy Link
Copy Source

Think I was waiting for design and/or direction. See coop-cloud/abra#361 (comment). Will see how that discussion pans out and re-instate something.

Think I was waiting for design and/or direction. See https://git.coopcloud.tech/coop-cloud/abra/pulls/361#issuecomment-19436. Will see how that discussion pans out and re-instate something.
👍 1
p4u1 marked this conversation as resolved
cli/app/restore.go Outdated
@@ -32,4 +30,2 @@
Name: "restore",
Aliases: []string{"rs"},
Usage: "Run app restore",
ArgsUsage: "<domain> <service> <file>",
p4u1 commented 2024-01-11 15:28:43 +00:00
Owner
Copy Link
Copy Source

Is it possible to set a backup file (which was previously downloaded) here?

Is it possible to set a backup file (which was previously downloaded) here?
moritz commented 2024-01-11 17:28:30 +00:00
Member
Copy Link
Copy Source

At the moment the backupbot only support to restore from restic snapshots directly. I'm not sure if this must be implemented in the backupbot or if abra could do the restore completely by itself.

At the moment the backupbot only support to restore from restic snapshots directly. I'm not sure if this must be implemented in the backupbot or if abra could do the restore completely by itself.
p4u1 commented 2024-01-12 11:18:34 +00:00
Owner
Copy Link
Copy Source

Yes, I guess the backupbot does not necessarily need to do a manual backup. This can also be implemented in abra. @decentral1se Maybe the code path for restoring from a local backup should be kept.

Yes, I guess the backupbot does not necessarily need to do a manual backup. This can also be implemented in abra. @decentral1se Maybe the code path for restoring from a local backup should be kept.
decentral1se commented 2024-01-12 15:50:28 +00:00
Author
Owner
Copy Link
Copy Source

@p4u1 @moritz looking at this again, I'm not sure how to proceed. There is still some outstanding discussion re: design of restore? See coop-cloud/backup-bot-two#26 and coop-cloud/backup-bot-two#42? We're already wildly over budget for this work, so not sure what can be done beyond what has been implemented now. Let me know what you think.

@p4u1 @moritz looking at this again, I'm not sure how to proceed. There is still some outstanding discussion re: design of restore? See https://git.coopcloud.tech/coop-cloud/backup-bot-two/issues/26 and https://git.coopcloud.tech/coop-cloud/backup-bot-two/issues/42? We're already wildly over budget for this work, so not sure what can be done beyond what has been implemented now. Let me know what you think.
p4u1 commented 2024-01-12 16:40:05 +00:00
Owner
Copy Link
Copy Source

Maybe we need a meeting to discuss this? I think the feature to restore from a local backup is pretty important. And I guess creating a backup to the local machine should also still be possible.

Maybe we need a meeting to discuss this? I think the feature to restore from a local backup is pretty important. And I guess creating a backup to the local machine should also still be possible.
👍 1
decentral1se marked this conversation as resolved
pkg/service/service.go Outdated
@@ -16,1 +16,4 @@
// GetService retrieves a service container based on a label. If prompt is true
// and the retrievd count of service containers does not match 1, then a prompt
// is presented to let the user choose. A count of 0 is handled gracefully.
p4u1 commented 2024-01-11 15:19:25 +00:00
Owner
Copy Link
Copy Source

" A count of 0 is handled gracefully." What does this mean? If I understood the code, an error gets returned, when no service was found.

" A count of 0 is handled gracefully." What does this mean? If I understood the code, an error gets returned, when no service was found.
p4u1 marked this conversation as resolved
pkg/service/service.go
@@ -17,0 +27,4 @@
return swarm.Service{}, fmt.Errorf("no services deployed?")
}
var backupServices []swarm.Service
p4u1 commented 2024-01-11 15:17:42 +00:00
Owner
Copy Link
Copy Source

The variable name should not contain "backup", since this is function is not specific to backup right?

The variable name should not contain "backup", since this is function is not specific to backup right?
p4u1 marked this conversation as resolved
pkg/upstream/container/exec.go Outdated
@@ -15,2 +15,3 @@
func RunExec(dockerCli command.Cli, client *apiclient.Client, containerID string, execConfig *types.ExecConfig) error {
func RunExec(dockerCli command.Cli, client *apiclient.Client, containerID string,
execConfig *types.ExecConfig) (io.Writer, error) {
p4u1 commented 2024-01-11 15:16:12 +00:00
Owner
Copy Link
Copy Source

Can you add a doc comment for this function and explain what the io.Writer is that gets returned?

Can you add a doc comment for this function and explain what the io.Writer is that gets returned?
p4u1 commented 2024-01-11 15:30:02 +00:00
Owner
Copy Link
Copy Source

Also is the returned io.Writer used anywhere? I could not find any usage of the first return value

Also is the returned io.Writer used anywhere? I could not find any usage of the first return value
decentral1se commented 2024-01-12 16:15:59 +00:00
Author
Owner
Copy Link
Copy Source

So far the out is ignored but I think we'll eventually want it. It's a pretty involved refactor (simple, but changes a few things in a few places), so I'd rather leave it as is for now.

So far the `out` is ignored but I think we'll eventually want it. It's a pretty involved refactor (simple, but changes a few things in a few places), so I'd rather leave it as is for now.
👍 1
p4u1 marked this conversation as resolved
p4u1 commented 2024-01-11 15:39:39 +00:00
Owner
Copy Link
Copy Source

I'm really looking forward to the backupbot integration in abra! I didn't test it localy yet. Just added a few comments to the code :)

I'm really looking forward to the backupbot integration in abra! I didn't test it localy yet. Just added a few comments to the code :)
🚀 1
decentral1se added 6 commits 2024-01-12 16:15:09 +00:00
fixup! feat: backup revolution 3ff37ddb87
fixup! feat: backup revolution 6a7e4973d7
fixup! feat: backup revolution 9cab8b5e59
fixup! feat: backup revolution daeba70dac
fixup! feat: backup revolution c63e97385f
fixup! feat: backup revolution
continuous-integration/drone/pr Build is passing
Details
continuous-integration/drone/push Build is passing
Details
f7ded7ef87
p4u1 reviewed 2024-01-12 16:35:40 +00:00
cli/app/backup.go
@@ -135,3 +75,1 @@
if !ok {
logrus.Fatalf("no backup config for %s? does %s exist?", serviceName, serviceName)
}
targetContainer, err := internal.RetrieveBackupBotContainer(cl)
p4u1 commented 2024-01-12 16:35:40 +00:00
Owner
Copy Link
Copy Source

I think backupContainer is a better variable name here and at the other occurrences

I think `backupContainer` is a better variable name here and at the other occurrences
decentral1se marked this conversation as resolved
decentral1se commented 2024-01-12 20:52:24 +00:00
Author
Owner
Copy Link
Copy Source

Restore functionality needs to be worked out, a meeting is in the works... for another PR!

Restore functionality needs to be worked out, a meeting is in the works... for another PR!
decentral1se merged commit f7ded7ef87 into main 2024-01-12 20:52:34 +00:00
decentral1se deleted branch backup-revolution 2024-01-12 20:52:34 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
moritz
p4u1
Labels
Clear labels
bug
Something is not working
 build
go build related issues
 ci/cd
Building things with CI/CD
 critical fix
https://docs.coopcloud.tech/federation/resolutions/passed/010/
 design
UI/UX
 documentation
Documenting all the things
 duplicate
This issue or pull request already exists
 easy-first-issue
Something for new people to get stuck into. We hope it's easy!
 enhancement
New feature
 help wanted
Need some help
 i10n
Everything to do with localisation
 i18n
Everything to do with internationalisation
 installer
Everything to do with the install script.
 invalid
Something is wrong
 question
More information is needed
 release
Release management
 release-candidate
Related to the new release candidate
 security
Security related
 tech-debt
test
Unit/integration testing
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
3wordchant aadil (Aadil Ayub) abra-bot (Abra Bot) ammaratef45 amras (Sarma) Apfelwurm BornDeleuze Brooke carla cas (Cassowary) coopcloud cyrnel decentral1se (d1) dede devydave fauno (fauno) iexos jade (Jade Ambrose) jjsfunhouse jmakdah2 (Jackie Makdah) joe-irving (Joe Irving) kawaiipunk (KawaiiPunk) knoflook kolaente lambdabundesverband linnealovespie (April) moosemower moritz notplants oxaliq (sorrel) p4u1 pharaohgraphy (Andrew 🐦‍🔥❤️‍🔥✴️) renovate-bot (Comrade Renovate Bot) ripclap simon sixsmith (Sixsmith) stevensting trav (Trav Fryer) val (val (he/him)) yksflip
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: toolshed/abra#361