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

storage/kubernetes: Removing Kubernetes TPR support #1517

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

venezia
Copy link
Contributor

@venezia venezia commented Aug 13, 2019

Part of fixing #1513 - this has a significant amount of code change and wanted to push this through before adding a flag to CRD creation.

Copy link
Member

@bonifaido bonifaido left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bonifaido bonifaido requested a review from srenatus August 14, 2019 09:43
@srenatus srenatus requested a review from JoelSpeed August 14, 2019 09:44
@srenatus
Copy link
Contributor

I'll defer to @JoelSpeed on this one 😉

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

No one should be using TPRs anymore as Kubernetes introduced CRDs about 2 years ago. I would however be tempted to leave the migration instructions for 1 release after we remove the code. If you haven't already migrated and we remove the migration instructions, you'll be a bit stuck!

Can we keep the migration instructions and add a TODO to remove them after the next release?

@venezia
Copy link
Contributor Author

venezia commented Aug 14, 2019

No one should be using TPRs anymore as Kubernetes introduced CRDs about 2 years ago. I would however be tempted to leave the migration instructions for 1 release after we remove the code. If you haven't already migrated and we remove the migration instructions, you'll be a bit stuck!

Can we keep the migration instructions and add a TODO to remove them after the next release?

In this PR there is a link in the storage docs to v2.17.0 as the last version to support TPR. If we're removing support for TPR in the next release, perhaps instead of keeping the migration instructions in the current documentation we simply add another line with a link to the old migration documentation ... and either remove scripts/dump-tprs or have that in the TODO to remove.

I could see it either way, just let me know

@JoelSpeed
Copy link
Contributor

@venezia You make a very good point, how about we just add a link to the old migration documentation saying something like "Note: If you have not migrated away from TPRs, please see the [documentation] and migrate before upgrading Dex"

Third Party Resources (TPR) have been removed from Kubernetes for
roughly 2 years.  This commit removes the support dex had for them.

Documentation has been updated to reflect this and to instruct users
on how to migrate from TPR-powered dex environment to a Custom Resource
Defintion (CRD) based one that dex > v2.17 will support
@venezia
Copy link
Contributor Author

venezia commented Aug 14, 2019

@JoelSpeed I updated the documentation a bit, removed the legacy script, squashed commits and force pushed back to my branch.

LMK if this works for you - thanks!

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Yep, I'm happy with this now, thanks for fixing it up!

@JoelSpeed JoelSpeed merged commit ab08d7b into dexidp:master Aug 14, 2019
@srenatus
Copy link
Contributor

Just for the record, I've been publishing a release while you've been discussing this. So, whatever was true for TPR in 2.17.0 probably is true for 2.18.0 as well 😄

@venezia
Copy link
Contributor Author

venezia commented Aug 14, 2019

hah - well that's the breaks :)

@JoelSpeed
Copy link
Contributor

Just for the record, I've been publishing a release while you've been discussing this. So, whatever was true for TPR in 2.17.0 probably is true for 2.18.0 as well 😄

I don't think this should cause an issue, the docs have been removed and people should notice it stopping working eventually 🤷‍♂

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
storage/kubernetes: Removing Kubernetes TPR support
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.

4 participants