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

Sanitize whitespace from config objects. #43

Merged
merged 3 commits into from
Jan 31, 2020

Conversation

blimmer
Copy link
Collaborator

@blimmer blimmer commented Jan 29, 2020

This commit adds logic to recursively sanitize the configuration object before it's written out to disk. It trims leading and trailing whitespace from string values in arrays or objects.

Fixes #42

This commit adds logic to recursively sanitize the configuration object
before it's written out to disk. It trims leading and trailing
whitespace from string values in arrays or objects.

Fixes kevinschaich#42
@blimmer blimmer force-pushed the feature/remove-whitespace branch from c772709 to 901b3e3 Compare January 29, 2020 17:45
const sanitize = (chunk) => {
return _.reduce(chunk, (result, v, k) => {
if (_.isObject(v) || _.isArray(v)) {
result[k] = sanitize(v);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where we recursively call this function to sanitize nested objects and arrays.

@@ -101,10 +101,27 @@ const getConfigEnv = async options =>
options
)

const sanitizeConfig = config => {
// recurse configuration objects and arrays to remove whitespace
const sanitize = (chunk) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Throughout, I opted to use lodash convenience methods since you had already included the package. If you'd prefer I use native methods (e.g. Array.isArray, native .trim()) let me know and I can swap them out!

@blimmer blimmer requested a review from kevinschaich January 29, 2020 18:24
@blimmer blimmer mentioned this pull request Jan 29, 2020
@kevinschaich
Copy link
Owner

Edit looks good to me @blimmer – however we seem to have broken the Travis job (not sure why the exit code is 0 but it didn't finish successfully):

https://travis-ci.com/kevinschaich/mintable/jobs/281691792

@blimmer
Copy link
Collaborator Author

blimmer commented Jan 31, 2020

Hey @kevinschaich it looks like that problem is because the PR's building off of my fork and doesn't have your mintable.config.json checked into the Travis secrets. I get the same problem when I try to run the master branch locally:

 ~/code/mintable-fork   master  yarn mintable
yarn run v1.21.1
$ node ./src/scripts/mintable.js

Using config /Users/blimmer/code/mintable-test/mintable.config.json.
Note: The messages displayed below are automated and may contain duplicates.

  ✔ Fetching current config
  ✔ Fetching current config
  ✔ Fetching current config
  ✔ Writing config
  ✔ Writing config
  ✔ Writing default config
  ✔ Fetching current config
(node:38497) UnhandledPromiseRejectionWarning: Error: Missing Plaid "client_id"
    at new Client (/Users/blimmer/code/mintable-test/node_modules/plaid/lib/PlaidClient.js:13:11)
    at Object.<anonymous> (/Users/blimmer/code/mintable-test/src/lib/plaid.js:19:22)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:991:10)
    at Module.load (internal/modules/cjs/loader.js:811:32)
    at Function.Module._load (internal/modules/cjs/loader.js:723:14)
    at Module.require (internal/modules/cjs/loader.js:848:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at /Users/blimmer/code/mintable-test/src/scripts/mintable.js:88:28
    at processTicksAndRejections (internal/process/task_queues.js:94:5)
(node:38497) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:38497) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
✨  Done in 0.39s.

I opened #51 to make it actually fail the build. However, I'm not sure what the ideal fix would be for builds on forks.

blimmer added a commit to blimmer/mintable that referenced this pull request Jan 31, 2020
On failure, catch rejected promises and exit with a non-zero exit code
to break the build.
Relates to kevinschaich#43
kevinschaich pushed a commit that referenced this pull request Jan 31, 2020
Thanks @blimmer – On failure, catch rejected promises and exit with a non-zero exit code
to break the build.
Relates to #43
@kevinschaich
Copy link
Owner

Hey @blimmer thanks for that subsequent PR – I’ve also added you as a collaborator so please feel free to make your branches here rather than on a fork to avoid the Travis issue!

Will try to think of a workaround for that in the coming weeks. I might be able to just create a public “template” account for Mintable on each of the services so my API keys are not out in the open.

@kevinschaich kevinschaich merged commit 3422fd0 into kevinschaich:master Jan 31, 2020
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.

401 error caused by whitespace in Google Client ID / Secret input
2 participants