IX: Implement Bidder interface, update endpoint. #1569
Conversation
|
@bsardo and @SyntaxNode - please let us know if anything is required for your review here. We're ready for you guys to merge this into the next release as soon as it's reviewed. |
There was a problem hiding this comment.
Thank you very much for updating the IX bid adapter. I'm fine if you'd like to remove the legacy implementation entirely, but please understand publishers still using the legacy endpoint will no longer have access to IX demand.
We plan to remove the legacy endpoint soon-ish. We don't have a definite date yet.
|
@SyntaxNode Thank you very much for your review and comments. One more thing. We've decided to put the legacy adapter code back. Would you prefer it as a separate commit? |
|
@SyntaxNode We have a deployment freeze until January 4, 2021. Our senior management asked us not to deploy our new adapter code until the freeze is over. Please hold off merging this PR until then. Thank you. |
|
@SyntaxNode Did you have a chance to review my new commits to see if I've addressed all of your concerns? I understand that with your recent refactoring I'll have to rebase my work, but it'll be mostly mechanical. |
There was a problem hiding this comment.
I think it looks great. I commend you on the comprehensive code coverage. I have just a few relatively minor comments.
I would still recommend to return an error if createRequestData fails to be consistent with how you handle setSitePublisherId errors. This will provide the publisher with information about why their impression was dropped.
I'm going to hold off on giving an official approval to ensure this is not merged in before the end of your code freeze, as you requested.
* Remove verbose logging. * Encapsulate maxRequests. * Return json.Unmarshal errors. * Fix the bid response type for the multi-format bid requests. * Remove legacy leftoover.
…additional error.
ix-prebid-support
force-pushed
the
ix-bidder
branch
from
409bc71 to
78971f2
Compare
|
@SyntaxNode Happy New Year! We are ready. |
| thisImp.Banner.W = &format.W | ||
| thisImp.Banner.H = &format.H | ||
| if thisImp.Banner != nil { | ||
| thisImp.Banner.Format = []openrtb.Format{format} |
There was a problem hiding this comment.
Thank you for adding the nil check. Although we plan to remove the legacy code later this year, it's nice to fix the "broken windows" until then. In our newer adapters we would ask for the banner object to be shallow copied first due to pointer sharing, but this object is created by MakeOpenRTBGeneric earlier in this adapter so I expect we're fine.
|
Looks fantastic. Thank you very much. |
No description provided.