-
Notifications
You must be signed in to change notification settings - Fork 48
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
Replace lucky env module #655
Replace lucky env module #655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small comments on the env.cr config file, but overall this is awesome! I'll look at a new release for the LuckyEnv shard.
# Example: | ||
# ``` | ||
# LuckyEnv.add_env :staging | ||
# LuckyEnv.staging? # => false | ||
# ``` | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I really like this. The only thing I think that's missing is mentioning that the value comes from ENV["LUCKY_ENV"]
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've amended the original commit. See 6acd902.
src/web_app_skeleton/config/env.cr
Outdated
@@ -1,20 +1,29 @@ | |||
module Lucky::Env | |||
extend self | |||
# Lucky manages environments using `LuckyEnv`. By default, development, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a link to the lucky_env github repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the documentation in 6acd902. However, instead of linking to the GitHub repository, I link to the configuration section of the Lucky documentation. I thought that would be more helpful. What do you think? 🙂
c9b9931
to
5ffc579
Compare
I've released v0.1.3 of LuckyEnv. No shard.yml update should be needed. I think it'll just pick it up. |
@jwoertink thank you! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
This attempts to replace the
Lucky::Env
module withLuckyEnv
. I was not entirely sure how we want to go about structuring theenv.cr
file. So any feedback is appreciated. 🙂Closes #654.