Skip to content
This repository has been archived by the owner on Apr 21, 2020. It is now read-only.

Improvements #9

Merged
merged 1 commit into from
Sep 3, 2015
Merged

Improvements #9

merged 1 commit into from
Sep 3, 2015

Conversation

theopak
Copy link
Collaborator

@theopak theopak commented Aug 26, 2015

Please do not merge yet. I'm actively working on this PR, and opening it now so that Travis-CI + Coveralls checks everything correctly. UPDATE: I'm done. Looks ok?

TODO: Version 1.5.9

  • Replace all instances of concatenation with path.sep with use of .path.join() or path.resolve() instead.
  • Ignore non-config files in config directory
  • Add test cases for having dot files in config directory.
  • Add test cases for having non-json, non-xml files in config directory.
  • Update doc in README
  • Use snake_case_because_it_matches_existing_code

@theopak theopak self-assigned this Aug 26, 2015
@@ -23,7 +23,7 @@ exports.createKey = function(root_dir, from_port, to_port, key_id, secret, cb) {
/* istanbul ignore if */
if (err) return cb(err);

var keyfile = keystore_path + path.sep + key_id;
var keyfile = path.join(keystore_path, key_id);
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 one instance in the PR where "/var" + path.sep + "logs/" concatenation is replaced by the platform-agostic path.join()function.

@theopak
Copy link
Collaborator Author

theopak commented Sep 2, 2015

@rb when you get a chance, please let me know if this looks good to merge. I'll squash the commits now.

@ryanbreen
Copy link
Contributor

Yep, I'll try to take a closer look today. I'm not super particular about commit history, so no need to squash.

    - Fix process.env var name in readme. It's OAUTH_REVERSE_PROXY_CONFIG_DIR not ()_PATH.
    - Use path.join()
    - Bump version to 1.5.9
    - Minor: indents and readme
    - Require config files to end in '.json'
    - Reject dot files in any keystore.
    - Config files can be xml, case insensitive
    - More "Getting Started" info in readme
    - Support relative paths in config vars
    - Let logger format error stacks itself.
    - Better support for relative path names.
    - Use snake_case for local var names.
    - Undo log format changes (using format strings again).
    - Add test resources for abnormal config filenames.
    - Add cases to test that ProxyManager does not load disallowed filenames
    - !fixup
    - More snake_case
    - Fix indents
    - Fix test should syntax
@theopak
Copy link
Collaborator Author

theopak commented Sep 2, 2015

Already squashed everything, I don't even know if it's possible to undo a force push. Enjoy your aggressively clean commit history.

fs.readdir(CONFIG_DIR, function(err, files) {
/* istanbul ignore if */
if (err) return cb(err);

// Require a '.json' or '.xml' file ending and reject dotfiles
Copy link
Contributor

Choose a reason for hiding this comment

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

We only support json. Any reason to include xml config files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I don't remember what I was thinking. I'll fix this line. I'll also remove the TODO comment since it's done.

@ryanbreen
Copy link
Contributor

Looks great, thanks!

ryanbreen added a commit that referenced this pull request Sep 3, 2015
@ryanbreen ryanbreen merged commit fbf614e into Cimpress-MCP:master Sep 3, 2015
@ryanbreen
Copy link
Contributor

1.5.9 published

@theopak theopak mentioned this pull request Sep 3, 2015
@theopak theopak mentioned this pull request Sep 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants