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

Remove MINIO_IDENTITY_OPENID_REDIRECT_URI - Deprecated #1720

Closed

Conversation

cniackz
Copy link
Contributor

@cniackz cniackz commented Aug 11, 2023

@cniackz cniackz self-assigned this Aug 11, 2023
@cniackz cniackz changed the title Removing deprecated var Removing deprecated MINIO_IDENTITY_OPENID_REDIRECT_URI var Aug 11, 2023
@cniackz cniackz changed the title Removing deprecated MINIO_IDENTITY_OPENID_REDIRECT_URI var Removing deprecated MINIO_IDENTITY_OPENID_REDIRECT_URI Aug 11, 2023
@cniackz cniackz changed the title Removing deprecated MINIO_IDENTITY_OPENID_REDIRECT_URI Remove MINIO_IDENTITY_OPENID_REDIRECT_URI - Deprecated Aug 11, 2023
Copy link
Contributor

@cesnietor cesnietor left a comment

Choose a reason for hiding this comment

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

how is it deprecated?
I still see it being used in the code:
https://github.com/minio/operator/blob/master/api/tenant-handlers.go#L409

@feorlen
Copy link
Contributor

feorlen commented Aug 11, 2023

Could the example use MINIO_BROWSER_REDIRECT_URL instead? Or is that not something you can do in a Helm chart. (I don't fully understand the linked issue.)

@cniackz
Copy link
Contributor Author

cniackz commented Aug 11, 2023

@feorlen it is in the documentation: https://min.io/docs/minio/linux/reference/minio-server/minio-server.html#envvar.MINIO_IDENTITY_OPENID_REDIRECT_URI the idea is to remove all legacy things as this var is no longer needed for the configuration.

@feorlen
Copy link
Contributor

feorlen commented Aug 11, 2023

remove all legacy things as this var is no longer needed for the configuration.

If MINIO_BROWSER_REDIRECT_URL is a direct replacement for the deprecated MINIO_IDENTITY_OPENID_REDIRECT_URI might be nice to include it in the example. (But not if the new envvar does something very different.)

Or perhaps it isn't a regularly needed option anyway. One thing we do in the docs is make examples with all the possible things you can set, then say "minimum required: A, B, C." Something to consider.

@cniackz cniackz force-pushed the adjust-helm-chart-for-deprecated-var branch from 08c7c82 to e89d27c Compare August 11, 2023 21:26
@cniackz
Copy link
Contributor Author

cniackz commented Aug 11, 2023

Yes @feorlen that sounds good to me. In the meantime, the best example I can think of is: https://github.com/minio/wiki/wiki/How-to-test-MinIO-SSO-keycloak

@cniackz
Copy link
Contributor Author

cniackz commented Aug 11, 2023

Hello @cesnietor it is deprecated because console is adding this for us:

./pkg/auth/idp/oauth2/provider.go:	redirectURL := scheme + "://" + r.Host + "/oauth_callback"

So we no longer need to provide this deprecated variable anymore.

But anyway you are right, I was missing other spots. I am changing those but I need to test them prior another review.

@cniackz cniackz force-pushed the adjust-helm-chart-for-deprecated-var branch from e89d27c to bc09943 Compare August 13, 2023 18:27
@cniackz
Copy link
Contributor Author

cniackz commented Aug 13, 2023

I will follow in #1722

@cniackz cniackz closed this Aug 13, 2023
@cniackz cniackz deleted the adjust-helm-chart-for-deprecated-var branch August 13, 2023 19:17
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.

3 participants