Skip to content

Make fetching HttpClient optional from context in PKCE auth flow#4295

Merged
pmahindrakar-oss merged 4 commits into
masterfrom
fix-pkce-auth
Oct 24, 2023
Merged

Make fetching HttpClient optional from context in PKCE auth flow#4295
pmahindrakar-oss merged 4 commits into
masterfrom
fix-pkce-auth

Conversation

@pmahindrakar-oss

@pmahindrakar-oss pmahindrakar-oss commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Tracking issue

Describe your changes

Previous PR introduced authentication with proxy http Client but it added a hard dependency to fetch this from the context .
This causes regression in cases the context is not set with this object.

Current change makes it backward compatible by using the default http client in case the context is not set with it.

Check all the applicable boxes

Tested this change locally by pulling this commit as saw authentication working fine in the broken environments

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
Comment thread flyteidl/clients/go/admin/pkce/auth_flow_orchestrator.go
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
eapolinario
eapolinario previously approved these changes Oct 24, 2023
@codecov

codecov Bot commented Oct 24, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4fc4988) 58.73% compared to head (8598033) 78.96%.

❗ Current head 8598033 differs from pull request most recent head 2a061f7. Consider uploading reports for the commit 2a061f7 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4295       +/-   ##
===========================================
+ Coverage   58.73%   78.96%   +20.23%     
===========================================
  Files         583       18      -565     
  Lines       50331     1298    -49033     
===========================================
- Hits        29562     1025    -28537     
+ Misses      18396      209    -18187     
+ Partials     2373       64     -2309     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
...dl/clients/go/admin/pkce/auth_flow_orchestrator.go 66.00% <100.00%> (ø)

... and 600 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
@pmahindrakar-oss
pmahindrakar-oss marked this pull request as ready for review October 24, 2023 22:32

@eapolinario eapolinario left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the unit test.

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