Skip to content
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

docs(lti11): Use consistent module path for LTI11Authenticator #112

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

BenGig
Copy link
Contributor

@BenGig BenGig commented Aug 18, 2022

I found a small inconsistency in the README. AFAIK the authenticator class in the example at line 106 should be tiauthenticator.LTI11Authenticator, as in the note below the example.

@jgwerner
Copy link
Collaborator

Hi @BenGig, thanks for the contribution! We had kept the LTIAuthenticator alias to maintain backward compatibility since the legacy version did not have LTI 1.1 and LTI 1.3. Therefore you can specify either LTIAuthenticator or LTI11Authenticator in the authenticator's configuration. The alias configuration is defined here.

To avoid confusion, perhaps we could update the documentation to reflect both of these options. @consideRatio @yuvipanda do you think?

@consideRatio
Copy link
Member

I think since the new suggestion requires 1.3.0, and 1.3.0 isn't available for example in the JupyterHub Helm chart yet for reasons, I figure we should stick with the old syntax for now and merge this after next release as its probably better that users are explicit with what they intend to use.

I'll make this a draft PR for now.

Action point

  • Await the next release, then merge.

@consideRatio consideRatio marked this pull request as draft November 10, 2022 00:15
@martinclaus
Copy link
Collaborator

@consideRatio I guess this can be merged now?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member

Absolutely, I'm on vacation until tomorrow. Feel free to merge if you consider this docs-only change is ready!

@martinclaus martinclaus self-assigned this Mar 30, 2023
@martinclaus martinclaus marked this pull request as ready for review March 30, 2023 09:53
@martinclaus martinclaus merged commit 1299ab8 into jupyterhub:main Mar 30, 2023
@martinclaus martinclaus changed the title Fix for inconsistency in README docs(lti11): Fix for inconsistency in README Mar 30, 2023
@martinclaus martinclaus changed the title docs(lti11): Fix for inconsistency in README docs(lti11): Use consistent module path for LTI11Authenticator Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants