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
Ravi/Aishwarya | Allow okta-config to be present outside the bundled jar #4
Conversation
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.
Overall I believe these changes make sense. Thank you for the PR. I have one small suggestion before merging.
src/ring/ring_okta/session.clj
Outdated
@@ -5,7 +5,7 @@ | |||
[ring.util.response :as response])) | |||
|
|||
(defn login [{:keys [okta-config-location params]}] | |||
(let [okta-response (saml/respond-to-okta-post (slurp (io/resource okta-config-location)) params)] | |||
(let [okta-response (saml/respond-to-okta-post (slurp (io/reader okta-config-location)) params)] |
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 believe the docs for clojure.java.io/reader
suggest:
Should be used inside with-open to ensure the Reader is properly closed.
https://clojure.github.io/clojure/clojure.java.io-api.html#clojure.java.io/reader
Is it possible for you to wrap the call in with-open
as the docs suggest?
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.
Good suggestion..
I have made the required changes
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.
Thanks @ravik-karn-tw. Looks good to me! I'll be merging and releasing 0.5.0
shortly.
Also, I made some minor code style changes on master. You may need to rebase your branch to clean up the conflicts. My apologies. |
7cd19d6
to
81b85b5
Compare
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.
Thank you for the feature suggestion and putting in the work to make it happen!
@ravik-karn-tw This has been merged and released as |
@bostonaholic Thanks for the help |
No description provided.