search

ddev / ddev-redis

Redis service for DDEV
Apache License 2.0
23 stars 8 forks source link

Respect disable_settings_management, fixes #6 #7

Closed joelpittet closed 2 years ago

joelpittet commented 2 years ago
hussainweb commented 2 years ago

The changes look good to me too. I agree about the output being confusing but it is no more confusing than many commands that report error twice. Yes, I wish it didn't happen but I can't think of a way to echo and not echo at the same time. :)

I am also thinking if this line is strictly required. Do other commands report such a message when disable_settings_management is true?

joelpittet commented 2 years ago

DDEV already tells the user about this, so maybe that would suffice and we can just exit 0? I'll push that change in here

rfay commented 2 years ago

Tested with ddev get https://github.com/joelpittet/ddev-redis/tarball/main and it behaved as expected with disable_settings_management=true. Then turned it off and made sure the existing settings were removed and did the get again and it worked as expected, created settings.

@hussainweb I'm fine with this when you are. Thanks @joelpittet !

hussainweb commented 2 years ago

I agree with you @rfay and merging it. I'll leave it up to you to check if you'd want to create a release for this change. I think it's worth creating a new release because it aligns with how DDEV handles settings.

rfay commented 2 years ago

Yes, needs a release. Probably anything that alters behavior needs a release; only tests or README changes don't. Thanks!