Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Conform to ConfigInitializable #9

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

mosamer
Copy link

@mosamer mosamer commented Jan 25, 2018

This PR adds support for initializing Bob, TravisCI and GitHub using Vapor configs. This allows an initialization process that is more inline with other Vapor projects and tutorials.

Instead of injecting the concrete type `Droplet`, use `ClientFactoryProtoco` instead. This applied to;
- GitHub client
- Travis CI
- SlackClient
- Bob
Conform to `ConfigInitializable` protocol to allow of initialization with `Config` inline with Vapor documentation. Following conformance implementation were added,
- Bob
- TravisCI
- GitHub
Copy link
Contributor

@i-dama i-dama left a comment

Choose a reason for hiding this comment

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

Great PR, thanks for your contribution 👍

Do you think it would make sense to move the config name "bob" into a constant?

@vincentaudoire
Copy link
Contributor

Hi @mosamer , many thanks for your PR 🎊
I'm having some concern about security when storing the configurations inside a json file in the repo. Currently we're getting the various tokens (github-access-token, slack-token ...) from the environment variable.
If we encourage users to store their configurations inside a .json file and store it on their github repo, wouldn't that be a huge security threat for them?

WDYT?

@mosamer
Copy link
Author

mosamer commented Jan 25, 2018

Hi @vincentaudoire, thanks for the review. I understand your security concerns but I feel they are already addressed.

  • As mentioned in the Readme update, the json file should be added to your .gitignore. This way you can avoid storing your credentials on Github.
  • If you still need to avoid adding the json file, you can pass these configs with CLI, using vapor run --config:bob.slack-token=<token>.

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

Successfully merging this pull request may close these issues.

3 participants