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.

Sign up for GitHub

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

Jump to bottom

MgidX: add optional region param #3220

Closed
wants to merge 18 commits into from
Closed

MgidX: add optional region param #3220

wants to merge 18 commits into from

Conversation

xmgiddev
Copy link
Contributor

gaudeamus and others added 18 commits May 17, 2019 12:41
new mgid adapter
d1ba998
Merge branch 'master' of https://github.com/prebid/prebid-server
6101f14 
# Conflicts:
#	exchange/adapter_map.go
#	openrtb_ext/bidders.go
increase coverage
08824f4
remove extra imports
2256c9a
Merge branch 'master' of https://github.com/prebid/prebid-server
515b471 
# Conflicts:
#	adapters/mgid/mgid.go
#	config/config.go
#	usersync/usersyncers/syncer.go
[mgid] optional placementId
11260e8
@evjman
Merge pull request #1 from prebid/master
ef1af9a 
Get last prebid-server changes
new adapter - MgidX
290fb01
change sync url
70cf06f
change sync url 2
8009199
@xmgiddev
Merge branch 'prebid:master' into master
271c114
change sync url 3
646888c
@xmgiddev
Merge branch 'prebid:master' into master
216f141
@xmgiddev
Merge pull request #2 from mgid/addMgidX
607a85e 
Add mgidX
fix import
9cf845f
change setting mediaType
be24fd0
@xmgiddev
add new required param host
189b7f2
@xmgiddev
fix conflicts
895735e
@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 895735e

mgidX

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:31:	Builder			80.0%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:44:	MakeRequests		83.9%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:98:	getImpressionExt	71.4%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:112:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:117:	makeRequest		78.6%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:145:	MakeBids		100.0%
github.com/prebid/prebid-server/adapters/mgidX/mgidX.go:184:	getBidMediaType		85.7%
total:								(statements)		86.0%

gargcreation1992
gargcreation1992 reviewed Oct 18, 2023
View reviewed changes
static/bidder-info/mgidX.yaml
@@ -1,4 +1,4 @@
endpoint: "https://us-east-x.mgid.com/pserver"
endpoint: "https://{{.Host}}.mgid.com/pserver"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not recommended. We highly discourage folks to use dynamic domain this way. Please review - #2606

Copy link
Contributor

Choose a reason for hiding this comment

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

@xmgiddev requesting to address above comment

gargcreation1992
gargcreation1992 requested changes Oct 18, 2023
View reviewed changes
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.

Please review #2606

@bsardo bsardo changed the title MgidX Bid Adapter: add new required param MgidX: add new required param Oct 23, 2023
@onkarvhanumante
Copy link
Contributor

@xmgiddev, prebid server had major release 2.0

requesting to update mgid:master with latest prebid master

@onkarvhanumante
Copy link
Contributor

@xmgiddev, prebid server had major release 2.0

requesting to update mgid:master with latest prebid master

@xmgiddev requesting to update mgid:master with latest prebid master and address PR comment

@onkarvhanumante
Copy link
Contributor

@xmgiddev, prebid server had major release 2.0
requesting to update mgid:master with latest prebid master

@xmgiddev requesting to update mgid:master with latest prebid master and address PR comment

@xmgiddev ping for updating this PR

bretg
bretg requested changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@bretg bretg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, but requiring users to enter a region is not acceptable.

I'll write up the solution in a more formal way but here's a summary:

  • document for host companies that you request regional endpoints
  • when they deploy your adapter's YAML file, they are responsible for using an appropriate regional endpoint

It is unrealistic to expect that multinational publishers can sort of which endpoint a given host should use. This places a heavy (or impossible) burden on the implementation of websites.

It's ok if host is an optional parameter for host companies that cannot support deploying region-specific endpoints.

@bretg bretg changed the title MgidX: add new required param MgidX: add optional host param Nov 17, 2023
@onkarvhanumante
Copy link
Contributor

onkarvhanumante commented Nov 20, 2023 •
edited
Loading

closing this inactive PR. Can be reopened when comments gets addressed

@xmgiddev xmgiddev changed the title MgidX: add optional host param MgidX: add optional region param Jan 3, 2024
@xmgiddev
Copy link
Contributor Author

xmgiddev commented Jan 3, 2024 •
edited
Loading

Sorry for not answering for a long time
@bretg we have changed the required HOST param to the optional REGION param
@onkarvhanumante we updated with latest prebid master, please reopen PR

@bretg
Copy link
Contributor

bretg commented Jan 4, 2024

@xmgiddev - please open a new PR from your fork.

@xmgiddev xmgiddev mentioned this pull request Jan 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onkarvhanumante onkarvhanumante onkarvhanumante left review comments

@gargcreation1992 gargcreation1992 gargcreation1992 requested changes

@bretg bretg bretg requested changes

Assignees

@gargcreation1992 gargcreation1992

@onkarvhanumante onkarvhanumante

@Sonali-More-Xandr Sonali-More-Xandr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xmgiddev @onkarvhanumante @bretg @gargcreation1992 @Sonali-More-Xandr @evjman