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

[FIX] Added headers to prompt service calls #173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Deepak-Kesavan
Copy link
Contributor

What

  • Added headers to prompt service calls
  • Fixed pre-commit issues

Why

  • To pass request id

How

...

Relevant Docs

Related Issues or PRs

Dependencies Versions / Env Variables

Notes on Testing

...

Screenshots

...

Checklist

I have read and understood the Contribution Guidelines.

Copy link
Contributor

@chandrasekharan-zipstack chandrasekharan-zipstack left a comment

Choose a reason for hiding this comment

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

NIT: If you're considering the suggestions update the other params accordingly and test it once

if not self.is_public_call:
headers = {"Authorization": f"Bearer {self.bearer_token}"}
default_headers = {"Authorization": f"Bearer {self.bearer_token}"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default_headers = {"Authorization": f"Bearer {self.bearer_token}"}
headers.update({"Authorization": f"Bearer {self.bearer_token}"})


def _post_call(
self,
url_path: str,
payload: dict[str, Any],
params: Optional[dict[str, str]] = None,
headers: Optional[dict[str, str]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
headers: Optional[dict[str, str]] = None,
headers: dict[str, str] = {},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chandrasekharan-zipstack Changing this will create a sonar issue sonarqube(python:S5717)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Deepak-Kesavan can you elaborate on the issue? Anyway my main idea was to avoid creating the default_headers dict in the function below and instead to attempt using the same headers arg to carry out the steps. Do you think that will be possible?

@@ -94,13 +109,19 @@ def _post_call(
"status_code": 500,
}
url: str = f"{self.base_url}/{url_path}"
headers: dict[str, str] = {}

default_headers = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default_headers = {}

Comment on lines +118 to +119
if headers:
default_headers.update(headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if headers:
default_headers.update(headers)

response: Response = Response()
try:
response = requests.post(
url=url, json=payload, params=params, headers=headers
url=url, json=payload, params=params, headers=default_headers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
url=url, json=payload, params=params, headers=default_headers
url=url, json=payload, params=params, headers=headers

@Deepak-Kesavan
Copy link
Contributor Author

@chandrasekharan-zipstack Keeping the code as it is because it violates a sonar rule.

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