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

Add Forgejo backup #310

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add Forgejo backup #310

wants to merge 13 commits into from

Conversation

ThisIsntTheWay
Copy link
Contributor

@ThisIsntTheWay ThisIsntTheWay commented Feb 12, 2025

Summary

  • Added: backup functionality to VSHNForgejo

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog

For what it's worth, this had no impact anyway as persistence is set to true by default
@ThisIsntTheWay ThisIsntTheWay marked this pull request as ready for review February 19, 2025 12:31
@ThisIsntTheWay ThisIsntTheWay requested a review from zugao February 20, 2025 13:07
Copy link
Collaborator

@zugao zugao left a comment

Choose a reason for hiding this comment

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

Some small changes

@@ -52,6 +52,9 @@ func AddK8upBackup(ctx context.Context, svc *runtime.ServiceRuntime, comp common
}

func createObjectBucket(ctx context.Context, comp common.InfoGetter, svc *runtime.ServiceRuntime) error {
if comp.GetName() == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be possible. The comp argument is the composite itself which cannot exist without a name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually did return an empty string on a few occasions.
With an empty string, it creates a bucket with a bad policy -bucket which minio complains about and then the whole pipeline gets stuck until that bucket is deleted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we have a deeper problem somewhere in the code.

@@ -52,6 +52,9 @@ func AddK8upBackup(ctx context.Context, svc *runtime.ServiceRuntime, comp common
}

func createObjectBucket(ctx context.Context, comp common.InfoGetter, svc *runtime.ServiceRuntime) error {
if comp.GetName() == "" {
return fmt.Errorf("could not get composition name")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's composite and not composition.

@@ -80,7 +80,7 @@ func NewRelease(ctx context.Context, svc *runtime.ServiceRuntime, comp InfoGette

release := &xhelmv1.Release{
ObjectMeta: metav1.ObjectMeta{
Name: comp.GetName(),
Name: resName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this has to change? This is in common package and will disrupt existing releases from other services, we have to be very carefully here.

return nil
}

func updateRelease(svc *runtime.ServiceRuntime, comp *vshnv1.VSHNForgejo) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method looks fine but I notice we copy paste it already 3 or 4 times. Would be nice if we could move it to common package.

@@ -0,0 +1,3 @@
#!/bin/bash

forgejo dump --type tar -t /tmp/backup -V -f -
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a new line here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants