feat: Add makeMaybe method to SslContextModule

[jakubjanecek]

It is sometimes necessary to turn off the initialization of SSL context to actually turn off SSL functionality of some library (e.g. for testing). This method makes it more simple by having an enabled flag in the configuration.

[sideeffffect]

The potentially problematic aspect of this solution is that if the caller uses make, it will always create an SSLContext (right?), even if enabled is set to false in configuration.

That's quite surprising and probably not what we want. Any suggestions how to remedy that?

We could throw in make if enabled is false 🤷‍♂️

[hanny24]

Shouldn't you update reference.conf as well?

[sideeffffect]

huh, the bot is really happy with merging stuff 😄
so what do you say @jakubjanecek
shouldn't we somehow remedy the issue I pointed out above before we cut a release?

[jakubjanecek]

@hanny24 I could add it but I see it as an extra feature not everyone needs. reference.conf is for the reference values of the ssl-config library and I made the decision to add an extra enabled flag there just for this method - it's kind of "virtual".

@sideeffffect Related to the think I say above, I don't want make to throw, there's no need for that, it always creates the context. The enabled flag is there just for this specific method and that's also why it's not in reference.conf.

I agree this is a small hack but it seems to me that it's not really harmful but can be very useful - I have the use case that's why I am adding it.

[jakubjanecek]

Of course we can revert this or update accordingly, the merge was premature - it's configured that once you have an approval you can merge. You shouldn't approve if you have doubts about anything.

[sideeffffect]

I don't want make to throw

it wouldn't be nice, but isn't it better to have at least some "safety" guarantee?
other option is to rename make -> makeAlways and makeMaybe to make -- but that's breaking compatibility, so probably not worth doing that
personally, I'm not sure what would be the best thing -- this PR is a hack, but creating an SSLContext if you don't want is quite less of a mistake than the opposite 🤷‍♂️

could others chime in? @tomasherman @jendakol @augi

[hanny24]

But you have just broken backwards compatibility.

[jakubjanecek]

@hanny24 How did I break it? Method make behaves exactly like it used to behave and it does not consider enabled at all (on purpose).

@sideeffffect Yep, that would be possible. I wouldn't worry about breaking compatibility right now because we are still not fully released. The point is that I think most people will use make and won't even know about enabled flag at all. Those who need it can use makeMaybe and they have to provide the enabled flag (which is not part of the standard ssl-config configuration) - nothing more.

[hanny24]

Ahh, sorry I thought it was changed method, not added.

[augi]

Breaking backward compatibility is not an issue IMHO if the change is important:

1. We are on 0.x versions.
2. We can increase the version accordingly.

[sideeffffect]

After thinking about this for a bit, I realized that the aspect of this PR that was bothering me was that the SSLContext is enabled by default, and you have to explicitly opt out of it, but you do that by using variable enabled (which you set to false). (The fact that it's called enabled is the important part.)

In general, I think there should be a rule that config variables should not be negatives, it makes for clearer reading, but maybe this case would be an exception from the rule.
How about naming the field disabled, and the methods make: F[SSLContext] and makeUnlessDisabled: F[Option[SSLContext]]?

the name ...UnlessDisabled hint at the fact that you would have to go out of your and disable it explicitly and you do that with a variable named disabled.

[sideeffffect]

Another option would be to backpedal on this PR, and resolve the problem @jakubjanecek described differently, possibly in (test I presume) code.
Since we're doing inversion of control (is that how it's called?), where we pass dependencies as arguments, it should be fairly easy -- at least in theory.

Or keep like it is... no biggie