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 context cancellation to backup routines #132

Merged
merged 1 commit into from
Apr 24, 2017

Conversation

eticzon
Copy link
Contributor

@eticzon eticzon commented Apr 21, 2017

This should get the ball rolling on issue #110. Please review 😄

I did notice that the project doesn't have an overarching test strategy/implementation in place, hence the reason for the lack of test(s).

I wanted to get your opinion on how to add (or start) adding some test suites onto the project @nilslice. I was thinking of maybe using something like https://github.com/spf13/afero for mocking the uploads/ directory. WDYT?

@nilslice
Copy link
Contributor

@eticzon - thank you! this looks great. I will give it a spin and get back to you with any changes.

Re: testing.. yes, we're desperately short on tests. I knew I would be working on the internal API and the interfaces a bit, and thus didn't want to slow down dev to keep tests up to date, but it's gotten a little out of hand ;)

Frankly, testing is easily the domain I have the least amount of experience in and would be hugely grateful to anyone who could step up to establish some starting point to get things going in the right direction for Ponzu's test strategy. I need to refactor quite a bit of the HTTP handler code to be testable, and I'm sure there is a lot elsewhere to fix up. The APIs are mostly ready to be frozen for an upcoming v1.0, so now is definitely a good time to start writing tests.

I looked at afero and it looks useful -- not only for the uploads but the CLI too. Commands new, add, build, and run in the CLI make use of certain directories on disk that could also benefit from testing with that package. If you'd like to use it, please go ahead! I am currently putting all** dependencies into vendor, flattening and checking them in (but removing their .git directory first to not submodule).

** except Bleve search, since it is a very large codebase.

@nilslice nilslice self-requested a review April 21, 2017 15:55
@eticzon
Copy link
Contributor Author

eticzon commented Apr 22, 2017

@nilslice cool! Thanks! Yup just @mention me if you need some changes done.

Re: tests, glad that you liked the afero idea. If it's OK with you, I'll write up a summary on a issue with a rough plan on getting started with testing. It'll take a while and by that time the handler code will be in an a more testable state like you said.

@nilslice
Copy link
Contributor

Re: tests, glad that you liked the afero idea. If it's OK with you, I'll write up a summary on a issue with a rough plan on getting started with testing.

That would be great, thank you 👍

It'll take a while and by that time the handler code will be in an a more testable state like you said.

Perfect. Thanks again for the help.

Copy link
Contributor

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

@eticzon - Tested on all 3 backup sources and manually killing the request triggered the cancellation. Thank you!

@nilslice nilslice merged commit 9bd1fb5 into ponzu-cms:ponzu-dev Apr 24, 2017
@eticzon
Copy link
Contributor Author

eticzon commented Apr 24, 2017

👍 Glad I was able to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants