Open robacarp opened 3 years ago
That's awesome you're integrating it. That should really help to configure Mosquito.
You can sort of do this now, but it's a little more code.
module Mosquito
class HabitatSettings
def self.default_redis_url
Log.warn {
<<-error_message
Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.
The functionality will be removed in the next _minor_ version bump, 0.9.0.
See: https://github.com/robacarp/mosquito#connecting-to-redis
error_message
}
(ENV["REDIS_URL"]? || "redis://localhost:6379")
end
end
Habitat.create do
setting redis_url : String = default_redis_url
end
end
Let me know if that would work for you. If so, then we can document this as an additional option.
I like it. Cracking open the HabitatSettings class isn't exactly terse, but it's a good short term workaround. Thanks
Another thing I just thought of is you can actually just move that method to mosquito itself and not have to open the HabitatSettings
class...
module Mosquito
def self.default_redis_url
Log.warn {
<<-error_message
Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.
The functionality will be removed in the next _minor_ version bump, 0.9.0.
See: https://github.com/robacarp/mosquito#connecting-to-redis
error_message
}
(ENV["REDIS_URL"]? || "redis://localhost:6379")
end
Habitat.create do
setting redis_url : String = Mosquito.default_redis_url
end
end
Actually, I think both of these will evaluate regardless of the default value being needed which means the warning message will be emitted always as well.
I think we should allow a default with a proc (I think it has to be a proc so you can specify a return type, but not sure). I think that will open up a lot of possibilities that are hard to do right now (like what you're proposing)
In this particular case though you could probably do what @jwoertink suggested with a tiny modification:
module Mosquito
def self.default_redis_url
Log.warn {
<<-error_message
Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.
The functionality will be removed in the next _minor_ version bump, 0.9.0.
See: https://github.com/robacarp/mosquito#connecting-to-redis
error_message
} if ENV["REDIS_URL"]? # Added the if so it only show if the ENV var is set
(ENV["REDIS_URL"]? || "redis://localhost:6379")
end
Habitat.create do
setting redis_url : String = Mosquito.default_redis_url
end
end
@paulcsmith yeah, that's nearly identical to what I landed on yesterday.
I also ran across the difficulty in testing the integration with Habitat around a default value. This may be because I don't have a proper mock in place for testing configuration values. I couldn't conceive a way to test that the default value existed and was used when the configuration parameter was blank, nor could I conceive a way to test that missing parameters actually cause an appropriate raise.
I have a hunch that I could subclass and scaffold some configuration-meta for testing that different configuration parameter sets result in the appropriate way, but I couldn't get there in an afternoon.
Nice! Glad you got it working
Thea ideal would probably be that Habitat has the feature built-in so you don't need to test it. Kind of like you probably wouldn't test that the getter
macro works and sets a default like it should.
I think if we add in the ability to accept a Proc and if the return type is the expected type then you should be good to go without testing, but maybe I am misunderstanding the use case?
BTW you can have Habitat raise if a setting is missing if that's what you were trying to test: https://github.com/luckyframework/habitat/blob/38cc672cfb353fcf52247ff65ed0ea408658d7ce/src/habitat.cr#L54
The reason for wanting the test is because that particular change is a full-stop-the-server-won't-work-anymore breaking change if the default value isn't in there -- the old behavior was implicit, like a lot of redis projects are, but the new behavior is implicit. My goal was to provide something which is backwards compatible for now, and raises a helpful warning, and is tested so that the backwards compatibility doesn't bitrot or otherwise evaporate without being noticed.
To put it another way, I trust Habitat is going to correctly apply my default value. I wanted a test that I correctly set the default value.
It's tricky because I already have the habitat config setup for the purpose of running the tests. The as yet undocumented testing helper is nice, but it doesn't provide a way to say "take the default value" like this imagine-code:
Mosquito.temp_config(redis_url: :default_value) do
assert_equal <whatever goes here>, Mosquito.settings.redis_url
end
And perhaps that is a usable solution? It would work in my case, but I can't speak to it being useful enough beyond my own purposes.
That makes sense! We should also document the temp_config because it is meant for exactly this purpose!
It is also super helpful for not just testing the settings but also overriding them in tests: https://github.com/luckyframework/lucky/search?q=temp_config
I think if that works in your case that should be good enough for now. But still agree on making a block a possible default!
Also if we allow a block/proc (probably a Proc) then the library can have a method for the proc like MyClass.default_redis_url
which returns the proc. Then you can test the proc separate from Habitat to see if it raises or returns what you expect. Does that make sense?
Yeah, I considered the MyClass.settings.default_config_var
method as well, and returning a proc sounds good to me.
The block value should probably work similar to the getter
/ property
macros where they are lazily evaluated. On top of that, the value should be memoized if it's not. I want to make sure that if your setting is creating some object (i.e. Redis connection), then it shouldn't be creating a new connection each time you call it. That maybe be tricky with the class level state, so maybe I open an issue separate on that.
Hey folks,
I'm integrating this over at robacarp/mosquito, which is currently taking configuration from an environment variable. In my current situation, it would be nice to be able to emit a message when a default value is rendered. In this case it would be a deprecation notice:
Maybe this would be otherwise useful, maybe not.