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

Revert repoze.who for ckanext-saml2 #9

Merged
merged 3 commits into from
Jan 15, 2020
Merged

Revert repoze.who for ckanext-saml2 #9

merged 3 commits into from
Jan 15, 2020

Conversation

adborden
Copy link
Contributor

@adborden adborden commented Jan 10, 2020

So because of a version conflict, we don't have a single set of dependencies that work for development and production for Inventory. For production, we use saml2 authentication with max.gov which requires repoze.who==1.0.18 and for local development we need user/pass authentication and repoze.who==2.0.

In catalog-app, we basically work around this by installing repoze.who==2.0 for develop, anad repoze.who==1.0.18 for staging/production.

The better fix would be to get off our forks for pysaml2 and ckanext-saml2 and make sure they work with recent versions of repose.who GSA/data.gov#414.

Or, maybe we try to get saml2 working in local develop, but that feels like another work around.

@adborden
Copy link
Contributor Author

In the short-term, to get CI passing, I think we'll want to install repoze.who==2.0 as part of the development/CI setup.

@adborden adborden requested a review from a team January 10, 2020 23:36
@adborden
Copy link
Contributor Author

@jbrown-xentity @FuhuXia what do you think about this work around and the long term fix?

@jbrown-xentity
Copy link
Contributor

So in my mind, this removed the possibility of letting pip overwrite the requirements-freeze.txt file. We would need to do a manual merge so as not to overwrite the comments and the repoze line. I can't think of a better way to manage this currently, just clarifying the process...

@adborden
Copy link
Contributor Author

Yes, this is true. And this is also the case with catalog-app. I will clarify this in the documentation.

Copy link
Contributor

@jbrown-xentity jbrown-xentity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving per discussion in comments. Will need to manage carefully.

@adborden
Copy link
Contributor Author

I added a note in the README.

@adborden adborden merged commit dc46729 into develop Jan 15, 2020
@adborden adborden deleted the bugfix/saml2 branch January 15, 2020 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants