Refactor construction of error-page proxy url to detention#94
Merged
Refactor construction of error-page proxy url to detention#94
Conversation
This was referenced Nov 26, 2015
Contributor
There was a problem hiding this comment.
in the other PR you check for shorthash, do you need to do same check here?
https://github.com/CodeNow/navi/pull/96/files#diff-7ccf25494e1cbc5cba89bfd84e901eb9R293
+ var findResult = mongo.constructor.findMasterPodBranch(naviEntry);
+ instanceShortHash = keypather.get(findResult, 'directUrlShortHash');
+ if (!instanceShortHash) {
+ log.warn(
+ logData,
+ 'getTargetHost redis.lrange mongo.fetchNaviEntry isBrowser !instanceShortHash masterpod');
+ return next(ErrorCat.create(404, 'Not Found'));
+ }
Contributor
Author
There was a problem hiding this comment.
Not here, our data * should * never exist in a state without 1 masterPodBranch. That other PR is checking for the event that the referer header is a valid formatted runnable elastic url but isn't actually a url that the target instance has an association with. Different situation. link you shared - that's an acceptable state that can occur.
…etention
* origin/master: (26 commits)
3.1.4
Removed async, because I'm not using it!
3.1.3
Fixed bad bug where the replace of {{DOMAIN}} was not being ran because the script wasn't replacing it's contents properly.
Renaming vars. Adding tests.
Removed counter
Fixed tests!
Removed trumpet dep
Updated logic to search for the first body or head tag.
Updated search logic to make it a bit easier to read.
Updated to use through2 logic!
Removed writing again!
Rolling back.
Bypass script adding for the moment.
Emitting end data.... Apparently?
Switched back to emit 'data'
Use this.queue instead of this.write. (testing a theory)
Emit 'data', data instead of data
Updated logic for the content stream.
Another method of handling injection of the script tag at the start of the head.
...
Conflicts:
lib/models/api.js
This reverts commit 689977a. Conflicts: lib/models/proxy.js
Contributor
|
this LGTM now, passing it over to @bkendall to see if his concerns were resolved |
Contributor
|
okay, I think the stuff I was looking at is fine now. have some merge conflicts, but resolve those up and you should be good to go. |
…etention * origin/master: 3.1.5 Use sinon assert match for method invocation argument testing Use sinon assertion and stub API with findAssociationShortHashByElasticUrl stubs Revert "Use sinon assertion and stub API with findMasterPodBranch stubs" Revert "Use sinon assertion and stub API with findAssociationShortHashByElasticUrl stubs" Use sinon assertion and stub API with findAssociationShortHashByElasticUrl stubs Use sinon assertion and stub API with findMasterPodBranch stubs Use sinon assertion and stub API with _processTargetInstance stubs Update tests and masterPod default routeing logic Fix naviEntry document selection for routing Add additional unit test for new logic Proxy to masterPod instance if no DNS mapping Conflicts: lib/models/api.js lib/models/mongo.js test/unit/api.js
cflynn07
added a commit
that referenced
this pull request
Dec 5, 2015
Refactor construction of error-page proxy url to detention
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea is to send the instance shortHash to detention via a query parameter in the proxy url so detention can query API for an up-to-date instance status and display the correct error message to the user. This PR will be paired with another detention PR