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 more "acceptable errors" on repo initialization #696

Merged
merged 3 commits into from
Jun 27, 2022

Conversation

jessebot
Copy link
Contributor

@jessebot jessebot commented Jun 23, 2022

Summary

This is to fix this issue: #690

We currently only check for "already intitialized", but in the restic backend for backblaze b2, they also throw "config already exists" as a possible error for the same thing. Because both errors are possible and probably fine, I created an array, in case more show up in the future, for easier fixing.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking,
    as they show up in the changelog
  • Update the documentation.
  • Update tests.
  • Link this PR to related issues.

@jessebot jessebot requested a review from a team as a code owner June 23, 2022 10:31
@jessebot jessebot requested review from bastjan and tobru and removed request for a team June 23, 2022 10:31
Signed-off-by: JesseBot <jessebot@linux.com>
Signed-off-by: JesseBot <jessebot@linux.com>
@jessebot
Copy link
Contributor Author

Closing because tests failed locally and I need to take a further look at this on my end before bothering maintainers. Sorry for the clutter.

@jessebot jessebot closed this Jun 24, 2022
…verse

Signed-off-by: JesseBot <jessebot@linux.com>
@jessebot jessebot reopened this Jun 27, 2022
@jessebot
Copy link
Contributor Author

jessebot commented Jun 27, 2022

Just need someone to take a peek and then I can take a peek at #563 as well, because it still shows up in tests, but happens before my fix as well.

Note: the e2e tests were not successful when I cloned master, so I've ignored them in testing this fix and instead tested directly with the method below.

I haven't tested restoring from backups yet, but I did test both schedules (on a two minute cron schedule) and backups (just deleting and reapplying the backup resource) and they're completing/marked as succeeded, as well as I checked backblaze b2 and the data is being updated. My testing was just running make docker-build/make docker-push with a modified Makefile to push to a local docker repository running inside a small KIND cluster, and then doing a helm upgrade with a values.yaml that changed the k8up image params to my local kind docker repo images. It failed on the changes from 3 days ago, but works on the most recent change I've pushed up to this branch in my forked repo.

EDIT: wrong issue, fixed to correct issue number, typos

@Kidswiss
Copy link
Contributor

hi @jessebot

I've triggered the pipeline here and the E2E tests seem to succeed.

Were you able to verify, that this works with B2? If so this looks good to me.

@jessebot
Copy link
Contributor Author

Yep, works with b2 for backups and schedules. Tested with annotated application aware backups for postgresql pvc.

@ccremer ccremer requested review from ccremer and Kidswiss and removed request for bastjan and tobru June 27, 2022 11:31
@ccremer ccremer added the bug Something isn't working label Jun 27, 2022
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

LGTM

@Kidswiss Kidswiss merged commit ccd553a into k8up-io:master Jun 27, 2022
@Kidswiss
Copy link
Contributor

@jessebot thanks for your contribution! 🎉

@ccremer ccremer changed the title change to array of str for acceptable errors on repo init Add more "acceptable errors" on repo initialization Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants