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

feat: helm chart oauth configuration #12868

Closed
wants to merge 1 commit into from

Conversation

vfoucault
Copy link

SUMMARY

Ability to configure Oauth for Helm

this is a revival of #10190

usage as follows in values.yaml

supersetNode:
  oauth:
    enabled: true
    registration_enabled: true
    userRegistrationRole: Public
    config:
      name: google
      whitelist:
        - '@email_domain.com'
      icon: fa-google
      token_key: access_token
      remote_app:
        api_base_url: https://www.googleapis.com/oauth2/v2/
        client_kwargs:
          scope: email profile
        request_token_url: null
        access_token_url: https://accounts.google.com/o/oauth2/token
        authorize_url: https://accounts.google.com/o/oauth2/auth
        client_id: bladibla_key
        client_secret: bladibla_secret

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Added oauth config within the helm chart.
@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #12868 (59d8714) into master (783aae1) will decrease coverage by 16.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12868       +/-   ##
===========================================
- Coverage   67.04%   50.82%   -16.23%     
===========================================
  Files        1022      476      -546     
  Lines       50186    17187    -32999     
  Branches     5204     4447      -757     
===========================================
- Hits        33648     8735    -24913     
+ Misses      16403     8452     -7951     
+ Partials      135        0      -135     
Flag Coverage Δ
cypress 50.82% <ø> (ø)
javascript ?
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/nativeFilters/ScopingTree.tsx 6.25% <0.00%> (-93.75%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx 11.76% <0.00%> (-88.24%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
superset-frontend/src/components/IconTooltip.tsx 13.33% <0.00%> (-86.67%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 2.94% <0.00%> (-85.95%) ⬇️
... and 878 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 783aae1...59d8714. Read the comment docs.

@amitmiran137
Copy link
Member

hi @vfoucault and thanks for your contribution
IMO we should add configurations in a more generic approach by adding different configs and not hardcoded in the YAML code
look in here: #13096

@vfoucault
Copy link
Author

hi @vfoucault and thanks for your contribution
IMO we should add configurations in a more generic approach by adding different configs and not hardcoded in the YAML code
look in here: #13096

Hum, i am quite perplexe about that because nothing is fixed here as you may specify your own oauth configuration that will be converted to json.

If you prefer to add a complex string into a yaml code, I wouldn't say that this is an improvement thought I must agree that the idea of a generic configuration overrider is quite good.

@vfoucault vfoucault closed this Jun 7, 2021
@vfoucault vfoucault deleted the feat/helm/oauth branch June 7, 2021 16:19
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.

Oauth should be configurable when deploying through helm
3 participants