Personally I would either call this option createRedisCacheLocally or create a few options so the hard-coded values you set can be configured:

services.gitea.redis.createLocally = true;
# something to configure db=0,pool_size=100,idle_timeout=180 like...
services.gitea.redis.database = 0;
services.gitea.redis.poolSize = 100;

I think my preference is to keep it simple with createRedisCacheLocally and let the user configure it themselves without this option if they need more customization (noting such in the description).

Why do I care about the name? useRedisCache sounds like it should be flexible enough to use a remote host. I like using the word Locally to indicate this is not the case.

Aside from that everything looks great. And keep in mind the naming is just my opinion. Maybe you disagree, in which case you can feel free to ignore me 😄

Aside from that everything looks fine from a quick glance.

ping @Ma27 for review - aren't you the maintainer of this module too? 🤔 If you're not you really should be 😄

I agree. Having more flexibility is probably a good idea.

Another way might be to match cfg.settings.cache.HOST and configure redis, memcached or other backends supported by gitea. I'm not sure if this might be too much hidden magic though. Setting an option in settings should really only add a setting to the configuration file and not have additional side effects. This is why having an option outside settings that configures something that just works would be preferable, I guess.

Exposing these settings in services.gitea.cache might be preferable, since gitea allows for multiple cache backends like Memcache and redis but afaik they are mutually exclusive. Caching to memory seems to be the default and does not take any extra options or configuration, so settings might be sufficient.

services.gitea.cache = {  
    enable = true; # default
    # Produces some thing like
    # HOST="redis://:macaron@127.0.0.1:6379/0?pool_size=100&idle_timeout=180s"
    "redis" =  {  # either "memory", "redis", or "memcache" with different options each
      user = "macaron"; # optional
      host = "127.0.0.1";
      port = 6379; # optional
      database = 0; # optional
      poolSize = 100; # optional
      idle_timeout = 180; # optional
    };
};

# This would produce 
# HOST="network=unix,addr=${redis.unixSocket},db=0,pool_size=100,idle_timeout=180"
services.gitea.cache = {
    enable = true;
    "redis".unixSocket = "/run/redis/redis.sock"; # mutually exclusive with (user, host, port)
}

For now, I'd like to only allow "redis" for the adapter, since Memcache might require additional configuration. Users can still set this up manually using the settings.

    "redis".unixSocket = "/run/redis/redis.sock"; # mutually exclusive with (user, host, port)

Or just "redis".useLocal = true and do the magic there.

services.gitea.cache.<name>.enable should always default to false, then we can leave out services.gitea.cache.<name>.unixSocket and just configure the unix socket as a sane default when services.gitea.cache."redis.enable = true. So either gitea.cache.memcache.enable or gitea.cache.redis.enable can be true.

That sounds reasonable 👍

I can leave a review later tonight or tomorrow @aanderse (this also applies to the other PRs where I got CCed :)).

ping @Ma27 for review - aren't you the maintainer of this module too? thinking If you're not you really should be smile

Since most of the modules don't have a meta.maintainers field (I don't really know about this guideline for modules tbh) I usually consider the package maintainers as responsible for the module as well unless stated otherwise... But yeah, guess I'll add myself there as well later :)

@GrahamcOfBorg test gitea

Still needs some more automated tests. The redis cache already works fine using the unix socket.

Still needs some more automated tests. The redis cache already works fine using the unix socket.

Should have sufficient test coverage now. Ready for review.

@GrahamcOfBorg test gitea

Is it possible to declare a type in a way that it can be a submodule with contents either like cache.redis, cache.memcached or cache.memory? Something similar to

cache = mkOption {
  type = myUnionType [ redisCache memcachedCache memory ];
};

where the list elements are the possible submodules.

I marked this as stale due to inactivity. → More info

I think you want to construct a generic option which has a option for the cache type instead.