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

DOM-40791 Integrate with Domino API Proxy #154

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

ddl-sean-floyd
Copy link
Contributor

@ddl-sean-floyd ddl-sean-floyd commented Oct 5, 2022

Link to JIRA

DOM-40791

What issue does this pull request solve?

Integrate the new Domino API Proxy into all API calls.

What is the solution?

Domino API Proxy is given precedence over all other auth types, both explicit, or implicit through environment variables.

Testing

Briefly describe how the change was tested. The purpose of this section is that a reviewer can identify a test gap, if any.

e.g. "I ran an upgrade from 4.2 to 4.6".

  • Unit test(s)

Pull Request Reminders

References (optional)

@ddl-sean-floyd ddl-sean-floyd requested a review from a team October 5, 2022 22:28
@ddl-sean-floyd ddl-sean-floyd force-pushed the sean.floyd/DOM-40791/api-proxy-integration branch from b6cffe5 to a1ebc09 Compare October 5, 2022 22:31
Copy link

@dmitriyvolk dmitriyvolk left a comment

Choose a reason for hiding this comment

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

Looks very reasonable

README.md Outdated Show resolved Hide resolved
domino/http_request_manager.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
def __init__(self, api_proxy):
self.api_proxy = api_proxy

def __call__(self, r):
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we update the header here as well, stating the auth method?

return r

def __initialize__(self, session):
session.proxies.update({
Copy link
Collaborator

Choose a reason for hiding this comment

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

or maybe here.

"""

if auth_token is not None:
if api_proxy is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if api_proxy: is also another option.

@ddl-olsonJD ddl-olsonJD self-requested a review October 6, 2022 19:24
Copy link
Collaborator

@ddl-olsonJD ddl-olsonJD left a comment

Choose a reason for hiding this comment

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

can we add some test for the proxy usage on here as well?
we can validate get_auth_by_type
and maybe test an api call using the api_proxy, with a 200 return?

@ddl-sean-floyd ddl-sean-floyd force-pushed the sean.floyd/DOM-40791/api-proxy-integration branch from 9082a2a to 2228b8a Compare October 7, 2022 22:20
@ddl-sean-floyd ddl-sean-floyd merged commit 691cd87 into master Oct 7, 2022
@ddl-sean-floyd ddl-sean-floyd deleted the sean.floyd/DOM-40791/api-proxy-integration branch October 7, 2022 22:20
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.

None yet

3 participants