-
Notifications
You must be signed in to change notification settings - Fork 32
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
RFS can now take the source snapshot #551
Conversation
Signed-off-by: Chris Helma <[email protected]>
Signed-off-by: Chris Helma <[email protected]>
/* | ||
* You have three options for providing the snapshot data | ||
* 1. A local snapshot directory | ||
* 2. A source host we'll take the snapshot from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another use case to potentially add in future, seems like would be to allow taking a local snapshot and using
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially; I thought about that as a option and wasn't able to think of a situation where I'd need it for testing but if you have something in mind I'm all ears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mind was around setting up a docker compose for local testing that had a low barrier for entry using a local snapshot. I'm worried having to setup an S3 bucket might turn some users away from initial trying the tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - yep, I could see that.
body.put("ignore_unavailable", true); | ||
body.put("include_global_state", true); | ||
|
||
// Register the repo; it's fine if it already exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems like it should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it provided value, but I guess it wasn't clear enough. Will re-write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed like it was saying this method would register the repo, but was pretty sure that happened in another method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, whoops, sorry - you're completely correct. I'll change that locally and it will flow out the next time I commit some code.
Signed-off-by: Chris Helma <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
============================================
- Coverage 76.57% 76.52% -0.05%
+ Complexity 1408 1407 -1
============================================
Files 155 155
Lines 6033 6033
Branches 543 543
============================================
- Hits 4620 4617 -3
- Misses 1047 1049 +2
- Partials 366 367 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
Issues Resolved
Testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.