-
Notifications
You must be signed in to change notification settings - Fork 742
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
New Adapter: AndBeyondMedia #2322
Conversation
@@ -0,0 +1,23 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-04/schema#", | |||
"title": "Compass Adapter Params", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an alias of the Compass adapter? If so, we can advise on better ways to create the alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode thanks corrected the title. This is not an alias, this is a separate adapter
Hi @AndBeyondMediaHB. Thanks for your contribution. We're curious why you've copied the entire Compass adapter including config files and tests. We strongly prefer avoiding duplicate adapter code. |
@bsardo The solution might be the same as we outsourced it's development to the same team.But please approve our adapter as it's real and we really need to use it asap, and it's working with our platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you've copied over the endpointId
parameter but it was omitted in the referenced docs PR. Can you please resolve this discrepancy?
{ | ||
"$schema": "http://json-schema.org/draft-04/schema#", | ||
"title": "AndBeyond.Media Adapter Params", | ||
"description": "A schema which validates params accepted by the Compass adapter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Compass
should be AndBeyondMedia
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,14 @@ | |||
maintainer: | |||
email: "[email protected]" | |||
capabilities: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a GVL ID for you in the vendor list. I just want to confirm that is the case, otherwise it should be specified here as gvlVendorID
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, we are not in the vendor list
} | ||
|
||
var invalidParams = []string{ | ||
`{"placementId": 42}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test case to invalidParams
of {}
where neither placementId
nor endpointId
are provided since the bidder-params file specifies oneOf
is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Local testing looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
doc - prebid/prebid.github.io#3899