-
Notifications
You must be signed in to change notification settings - Fork 22
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
Config folder #2
Conversation
README.md
Outdated
#### Config folder | ||
If you want to separate your environments by file | ||
* create a `config` folder in the root directory of your project | ||
* move the existing `config.yml` file to `config/defaults.config.yml` |
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 would change config/defaults.config.yml
to config/defaults.yml
README.md
Outdated
If you want to separate your environments by file | ||
* create a `config` folder in the root directory of your project | ||
* move the existing `config.yml` file to `config/defaults.config.yml` | ||
* create custom environments via `config/[env].config.yml` |
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.
likewise I would change config/[env].config.yml
to config/[env].yml
index.js
Outdated
return _.merge({}, | ||
loadConfig('config/defaults.config.yml'), | ||
loadConfig('config/' + getEnvId() + '.config.yml') | ||
) |
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 should be lined up with the r
index.js
Outdated
) | ||
} | ||
|
||
function loadConfig (file) { |
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 would change this to loadConfigFile
index.js
Outdated
let environments = config.environments || {} | ||
let envId = getEnvId() | ||
let ENVID = envId ? envId.toUpperCase() : undefined | ||
let environmentType = (_.includes(environments.static, envId) ? envId : undefined) || environments.default | ||
|
||
function loadConfig () { | ||
function determineConfigMode () { |
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 would change this to loadConfig
since it is actually loading the full config. see comment below for loadConfigFile
index.js
Outdated
)(args) || | ||
process.env.ENVIRONMENT_ID || | ||
getEnvIdFromBranch() | ||
var templ = singleFileMode ? useStaticFromConfig() : useFileList() |
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 misses the point of static
. static is meant to specify which environments you can deploy specifically using --prod --staging, etc. there are cases where you want to do this without having a specific environment section in your config or a separate file. the config.environments.static should be used whether it is single file or multi file. i would revert the code for getEnvId()
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.
so my intention for the config folder was to act as the static environment list, but all good. I'll write up some more tests to make sure that the env's still behave the same way
Closing, working on a better implementation |
No description provided.