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

Env var option as part of README #57

Merged
merged 5 commits into from
May 24, 2017
Merged

Conversation

CrowdHailer
Copy link
Contributor

No description provided.

@CrowdHailer
Copy link
Contributor Author

Question. Is it possible to use this setup for all settings? I would like to use it for the port and servername aswell

@kdisneur
Copy link
Contributor

kdisneur commented May 23, 2017

Good spot! You are right, we should add this information in the README.

Really, we should add it because the library not only supports this feature for username and password, it already supports it for all configuration fields.

What do you think of adding a more general sentence in the README about the support of {:system, "ENVIRONMENT_VARIABLE"} and also one in the documentation? 😁

username: "your.name@your.domain",
password: "pa55word",
username: "your.name@your.domain", # or {:system, "SMTP_USERNAME"}
password: "pa55word", # or {:system, "SMTP_PASSWORD"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use "SMTP_PASS" here? Or, update "SMTP_PASS" in the library documentation to use "SMTP_PASSWORD"?

We can use SMTP_PASSWORD, the naming is better but we would just like to keep all the example consistent. :)

@CrowdHailer
Copy link
Contributor Author

What do you think about that addition to the README

@kdisneur
Copy link
Contributor

Thanks - looks good to me! 👍

If you can also add this sentence to the library documentation and rebase your commits, we will be really happy to merge it

@CrowdHailer
Copy link
Contributor Author

develop is the default branch on your repository?

rebasing won't do anything, unless I am missunderstanding

@kdisneur
Copy link
Contributor

Sorry - you are right the default branch is develop. I meant squashing your commits.

@CrowdHailer
Copy link
Contributor Author

There is a squash and merge option on PRs is there not.

@kdisneur
Copy link
Contributor

That's right, I always forget about this feature :)

Anyway, we can't merge the PR right now because the build fails. Credo found this issue on your last commit:

Code Readability                                                              
┃ 
┃ [R] ↓ There should be no trailing white-space at the end of a line.
┃       lib/bamboo/adapters/smtp_adapter.ex:7:1 (Bamboo.SMTPAdapter)

@CrowdHailer
Copy link
Contributor Author

It's a shame these tools can't make formatting changes. I normally just use the github web interface when contributing to documentation stuff.

@kdisneur kdisneur merged commit 98eafe9 into fewlinesco:develop May 24, 2017
@kdisneur
Copy link
Contributor

Thanks for the Pull Request :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants