caddyserver / cache-handler

Distributed HTTP caching module for Caddy
Apache License 2.0
262 stars 19 forks source link

Proposal to use Souin as cache system #17

Closed darkweak closed 2 years ago

darkweak commented 3 years ago

As we discussed on Slack, here is the PR to use Souin as cache system. It respects the Caddy expectations, supports JSON and Caddyfile configuration formats, supports Olric as embedded mode or client mode and for those who don't need a distributed storage, it supports a non distributed one.

It closes the #16

Open to discuss with you about it.

darkweak commented 3 years ago

Update: I have to tag a new version on Souin with the embedded Olric support and the Ykeys. After that the CI will be green.

darkweak commented 3 years ago

Any update about the implementation @dunglas @mholt ?

mholt commented 3 years ago

I don't have a strong opinion either way. I think you guys (or someone else who's invested in the cache functionality) should take charge on this feature.

darkweak commented 3 years ago

@dunglas can you rerun the test suite please ?

dunglas commented 3 years ago

done

darkweak commented 3 years ago

Can you re-run again @dunglas ? Thanks to GH to block the CI for non-maintainer contributor and require an approval before each CI run.

darkweak commented 3 years ago

Should be ok now @dunglas 😅

darkweak commented 3 years ago

Ready to merge and tag on my side!

mholt commented 3 years ago

Sent you a collaborator invite -- that should give you the flexibility you need to help maintain this repo. :+1:

darkweak commented 3 years ago

Any update about the implementation/review @dunglas ?

mholt commented 3 years ago

@Darkweak I can't speak for @dunglas, but you are now a maintainer of the repo, so you could probably merge this when you're ready.

darkweak commented 3 years ago

@mholt I'm not very fan about merge without approve on a public/community repository. And that's not like a simple rewording. It will impact the core declaration of this handler 😅.

dunglas commented 3 years ago

Sorry I haven't had time to review this and I probably won't be able to before my holiday. So I agree with @mholt, let's move forward and merge.

darkweak commented 3 years ago

Wait for @dunglas feedbacks.

polarathene commented 3 years ago

Just curious, if this is merged to become the new cache-handler module, how does it differ from the caddy module from Souin?

Will they be effectively the same, or some advantage/disadvantage between the two?

darkweak commented 3 years ago

@polarathene this should be equal to the caddy module from Souin. The advantage of the Souin one would be the release delay between a new tagged version of the core and the caddy module. It will be available earlier in the Souin repository. And there are some discussions about block some parts such as the ykeys support in this module which will be available in Souin repository.

polarathene commented 3 years ago

this should be equal to the caddy module from Souin.

What is the purpose of separate module cache-handler? Official endorsement under the "caddyserver" organization?

Is it intended as a simpler config/API for those that don't need more advanced full featured version of Souin caddy module?

darkweak commented 3 years ago

What is the purpose of separate module cache-handler? Official endorsement under the "caddyserver" organization?

Yes that's it.

Actually the simplest configuration for this module using Souin would be the default ttl declaration as root directive. With that, it won't enable the Souin api, won't store any ykeys to group requests by ykey.

polarathene commented 3 years ago

Yes that's it.

Ah ok :)

It is to provide a simpler cache solution with smart defaults, like Caddy is well known for.

I think it would still be appreciated to better clarify the difference, than just stating it's based on Souin (since it offers it's own caddy module for more advanced features Souin has).

darkweak commented 2 years ago

🆙 @dunglas @mholt @francislavoie can you re-review while I'm re-implementing the tests please ? Thanks 🙂

mholt commented 2 years ago

Am pretty busy (and haven't been following this branch toooo closely, I admit) -- but let me know if there's something specific I can help out with.

darkweak commented 2 years ago

@dunglas tests are reimplemented

darkweak commented 2 years ago

Ping @dunglas for the last review ?

amanintech commented 2 years ago

Any new update when is this going to be out ? Seems like this is the only solutions where all nice helps have headed to

darkweak commented 2 years ago

We are waiting for a last code review but we don't know when @dunglas will have time.

francislavoie commented 2 years ago

If @dunglas is good with this, then let's do it!!

This has been a long time coming. Thanks for all the effort @darkweak 😁

mholt commented 2 years ago

Excellent work everyone, I'm excited to see this go forward! I'll tweet about it when I have a chance.

mattvb91 commented 2 years ago

Hi everyone thanks for getting this in!

I think im currently stuck trying to get the following config pieces in (using json api on caddy):

@souin-api path /souin-api*

cache @souin-api {}

So that I can then use that souin endpoint to purge keys. Is there anyone that could help me how to convert that into the json config? This is what i currently have:

"apps"  => [
                "cache" => [
                    'api'       => [
                        'basepath' => '/cache/',
                    ],
                    "log_level" => "info",
                    'ttl'       => '3600s',
                ],
]

But I am not sure where the actual souin endpoint is located at now so I can purge / see the keys in cache.

I have an issue open here: #26 would appreciate any pointers! Thanks