Conversation
waiter/src/waiter/core.clj
Outdated
hostnames (if (sequential? hostname) | ||
(clojure.set/union (set hostname) | ||
#{waiter-router-hostname waiter-router-ip}) | ||
(set [hostname waiter-router-hostname waiter-router-ip]))] |
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.
Nitpick: Can we use the set literal here?
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.
Done
waiter/src/waiter/core.clj
Outdated
(waiter-request?-factory (set [hostname waiter-router-hostname waiter-router-ip])))) | ||
waiter-router-ip (.getHostAddress local-router) | ||
hostnames (if (sequential? hostname) | ||
(clojure.set/union (set hostname) |
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.
clojure.set
is already imported, we can just do set/union
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.
Done
waiter-router-ip (.getHostAddress local-router)] | ||
(waiter-request?-factory (set [hostname waiter-router-hostname waiter-router-ip])))) | ||
waiter-router-ip (.getHostAddress local-router) | ||
hostnames (if (sequential? hostname) |
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.
+1 on use of sequential?
@@ -1099,3 +1099,14 @@ | |||
|
|||
(testing "require-authentication" | |||
(is (= {:source :spnego-handler} (request-handler {:headers {}}))))))) | |||
|
|||
(deftest test-waiter-request?-fn |
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.
The test should also include negative scenarios, e.g. (is (not (waiter-request?-fn {:headers {"host" "waiter-host-1"}})))
in the first testing block.
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.
Done
waiter/test/waiter/core_test.clj
Outdated
(let [config {:settings {:hostname "waiter-host"}} | ||
waiter-request?-fn ((:waiter-request?-fn routines) config)] | ||
(is (waiter-request?-fn {:headers {"host" "waiter-host"}})))) | ||
(testing "list hostname config" |
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.
Nitpick: separate these two testing
blocks with a whitepsace.
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.
Done
Why did we merge before a passing build? |
@@ -700,8 +700,12 @@ | |||
:waiter-request?-fn (pc/fnk [[:settings hostname]] |
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 isn't the only place hostname
is used.
No description provided.