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

Taboola Bidder Adapter: Change Endpoint #3058

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

ahmadlob
Copy link
Contributor

@ahmadlob ahmadlob commented Aug 27, 2023

changes in the server endpoint, using query params for publisher id and exchange params instead of path params

@ahmadlob ahmadlob changed the title Taboola Bidder Adapter - Change Endpoint To Static Taboola Bidder Adapter: Change Endpoint To Static Aug 27, 2023
@@ -1,4 +1,4 @@
endpoint: "https://{{.MediaType}}.bidder.taboola.com/OpenRTB/PS/auction/{{.GvlID}}/{{.PublisherID}}"
endpoint: "https://{{.MediaType}}.bidder.taboola.com/OpenRTB/PS/auction?exchange={{.GvlID}}&publisher={{.PublisherID}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on your description in the PR that this addresses the issue #2612, this endpoint is still using the dynamic host. You would need to use the static domain. For example, in this case, MediaType macro needs to be moved out as query param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gargcreation1992 It's not a path param, we currently have two endpoints for two different services that each serve a different media type ( native, display).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmadlob This is a fine change to make but this change definitely not addressing the issue #2612. For static host, macro should not be included in the endpoint domain.
The above host is still dynamic.
Please change the description of the PR to reflect the changes that you are doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmadlob Ping to please take a look at Ashish's comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gargcreation1992 changed the PR description and title

@ahmadlob ahmadlob changed the title Taboola Bidder Adapter: Change Endpoint To Static Taboola Bidder Adapter: Change Endpoint Sep 5, 2023
Copy link
Contributor

@gargcreation1992 gargcreation1992 left a comment

Choose a reason for hiding this comment

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

Thank you for changing the description. Approving it now!
Please be mindful that this issue does not address #2612

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.

3 participants