Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

fix: Clean providers list when oauth config is disabled #13

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

frascuchon
Copy link
Member

Description

When .oauth.yaml file contains enabled: false, the oauth endpoints return the configured provider info. This PR fixes it and set an empty list when enabled=False

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

Running locally and HF spaces

  • Test A
  • Test B

Checklist

  • I followed the style guidelines of this project
  • I did a self-review of my code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@frascuchon frascuchon self-assigned this Feb 2, 2024
@jfcalvo
Copy link
Member

jfcalvo commented Feb 5, 2024

Why is this different from commit argilla-io/argilla@5621cee ?

@frascuchon
Copy link
Member Author

Why is this different from commit argilla-io/argilla@5621cee ?

Even if both solutions fix the main problem in HF spaces with the default setup, this approach is more robust since the provider-related endpoints GET /oauth2/providers/:provider/... will return a 404 status. The solution in the other repo will try to trigger the authentication flow since the provider is registered.

@frascuchon frascuchon merged commit 0a6b1ea into main Feb 5, 2024
10 checks passed
@frascuchon frascuchon deleted the bugfixes/no-providers-when-oauth-is-disabled branch February 5, 2024 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants