Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a MOSQUITO_REDIS_URL Env Variable by default #39

Open
wants to merge 2 commits into
base: master
from

Conversation

@catgirlsec
Copy link
Contributor

catgirlsec commented Nov 17, 2019

The default REDIS_URL might be used by conflicting dependencies, or a app may wish to have a dedicated instance for background jobs. This pull request changes mosquito's behavior to check for a "MOSQUITO_REDIS_URL" ENV Var before using the "REDIS_URL" ENV Var.

@robacarp

This comment has been minimized.

Copy link
Owner

robacarp commented Nov 17, 2019

Hi @nsuchy thanks for the pull request. Ideally I'd like to ditch the magic URL altogether, as indicated by #3. What do you think about that?

@catgirlsec

This comment has been minimized.

Copy link
Contributor Author

catgirlsec commented Nov 17, 2019

Hi @nsuchy thanks for the pull request. Ideally I'd like to ditch the magic URL altogether, as indicated by #3. What do you think about that?

You're suggesting we create a config file / initializer system instead?

@catgirlsec catgirlsec force-pushed the catgirlsec:master branch from b236efb to 8cc9aa2 Nov 19, 2019
@catgirlsec

This comment has been minimized.

Copy link
Contributor Author

catgirlsec commented Nov 19, 2019

Fixed a syntax error in this pull request. I think a good answer to this question starts at whether we have a lot of options in configuration. Right now Mosquito is super simple background task runner. If it gets more features maybe a configuration file will be necessary. I think environment variables are enough for the time being.

@catgirlsec

This comment has been minimized.

Copy link
Contributor Author

catgirlsec commented Nov 19, 2019

Currently this change looks like it'll require us to use Mosquito.Base.settings.redis_url unless I'm misunderstanding something.

@catgirlsec catgirlsec force-pushed the catgirlsec:master branch from ad27a4e to 32e599c Nov 19, 2019
Copy link
Owner

robacarp left a comment

@nsuchy Thank you for jumping in here, I appreciate it a lot. 🍰 I left a few comments before I realized that you're not to completion yet. In general, please avoid unnecessary churn in the formatting of the code. It makes the pull request muddy, and harder to review. Thanks again!

assignment = assignment + " = #{parameter["value"]}" if parameter["value"]
assignment
end.join(", ").id
}})

This comment has been minimized.

Copy link
@robacarp

robacarp Nov 20, 2019

Owner

I'm sure this got pulled in by some automatic save, but unless any of these changes are functional they should be left out, please 😁

# when the value is not set.
#
# Use `String#dump` when you want the example to be wrapped in quotes
setting redis_url : String, example: "redis://localhost:6379"

This comment has been minimized.

Copy link
@robacarp

robacarp Nov 20, 2019

Owner

Should this be the default value as well?


authors:
- robacarp

crystal: 0.29.0
crystal: 0.31.0

This comment has been minimized.

Copy link
@robacarp
@@ -1,17 +1,19 @@
name: mosquito
version: 0.5.0
version: 0.6.0

This comment has been minimized.

Copy link
@robacarp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.