Update README to point to node-saml documentation
#886
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #886 +/- ##
=======================================
Coverage 64.42% 64.42%
=======================================
Files 4 4
Lines 149 149
Branches 37 37
=======================================
Hits 96 96
Misses 30 30
Partials 23 23 ☔ View full report in Codecov by Sentry. |
|
@markstos can you please have a look at this. It is a major change of the README, mostly deletes. I want to get another set of eyes on it. |
There was a problem hiding this comment.
As a user, I prefer if I don't have to use multiple references, but given the human resource contraints of open source maintenance, I understand this is reasonable way to keep the documentation maintenance load lower.
I've left some comments to suggest putting the two bits of documentation about passport-saml specific args next to each other and moved to the section on Configuring Strategy which is where they seem to apply most directly.
Otherwise, good to merge. 👍🏼
README.md
Outdated
|
|
||
| ## Installation | ||
|
|
||
| ```shell | ||
| npm install @node-saml/passport-saml | ||
| ``` | ||
|
|
||
| Please note that most of the configuration options for this are simply passed through to the underlying `node-saml` | ||
| library. For more details on the configuration options and how the underlying SAML flows work, please see the [node-saml documentation](https://github.com/node-saml/node-saml/blob/master/README.md) | ||
|
|
There was a problem hiding this comment.
This should be moved from "Installation" to "Configure Strategy" and bolded. "Please note that" can be dropped.
There was a problem hiding this comment.
Also, clarify the docs by instead of using this, say "for the Strategy constructor".
README.md
Outdated
| - `additionalParams`: dictionary of additional query params to add to all requests; if an object with this key is passed | ||
| to `authenticate`, the dictionary of additional query params will be appended to those present on the returned URL, | ||
| overriding any specified by initialization options' additional parameters (`additionalParams`, | ||
| `additionalAuthorizeParams`, and `additionalLogoutParams`) |
There was a problem hiding this comment.
This section, with the arguments that are processed by passport-saml and not node-saml, should be moved up to the top of the "Usage" section", just below disclaimer sentence that was added, linking to node-saml.
|
@markstos , I think I've addressed your concerns. Would you like to take another look? |
README.md
Outdated
|
|
||
| These are only the additional configuration parameters related to `passport-saml`. For the full list | ||
| of parameters, please see the | ||
| [node-saml documentation](https://github.com/node-saml/node-saml/blob/master/README.md) |
There was a problem hiding this comment.
I don't have solution for this but documentation of released passport-saml versions refers always to current head of development branch (master) of node-saml meaning that immediately after new major refactoring (to node-saml) there is a mismatch between node-saml module used by earlier passport-saml versions and documentation that those earlier passport-saml versions point. I.e. passport-saml 5.x readme at npmjs could refer to node-saml master branch which could contain unreleased/upcoming node-saml 10.x version etc.
There was a problem hiding this comment.
This really an internet-wide problem with software, where you have one version installed, but then look up the docs for the software on the web. Either you get the latest docs and not your current docs, or it's up to you to track down the docs for the version of the software you actually need.
In summary: I think this is "good enough" without further changes and is on-par with what other software projects do.
There was a problem hiding this comment.
Maybe if node-saml repository would contain branch per major version e.g. 5.x (and that branch would be created immediately after major release) and passport-saml side readme would refer to corresponding node-saml major version branch'es readme.
I.e. master branch (development branch) of passport-saml's readme would always refer to master branch (development branch) of node-saml.
Both repositories would contain major release branches (5.x, 6.x, ...) which are created immediately after/same time new major release and e.g. 6.x passport-saml's readme would be changed to point to 6.x node-saml readme etc.
There was a problem hiding this comment.
@srd90 , you're talking about "Release Branch Workflow" vs. "Continuous Delivery". We currently use a "Continuous Delivery" methodology, and I've used "Release Branch Workflow" before. I only think that partially solves the problem because you then have to make sure you keep the README up-to-date.
My goal is to make sure that we release passport-saml shortly after a release to node-saml to keep things in semver-major lock-step.
Update the README to point to
node-samldocumentation for configuration options that are passed through tonode-saml.