New adapter: Adagio

[osazos]

Add a new Prebid-Server (Go) adapter for Adagio (we already have a prebid.js adapter)

[hhhjort]

It may not be a bad idea to have a multiformat test. And a test or two to exercise that bad response paths in MakeBids().

[osazos]

hi @hhhjort, thanks for your review. I added some tests. Let me know if it is ok you.

[hhhjort]

Is there a reason you hardcoded some tests in by hand rather than adding more JSON tests?

[hhhjort]

It is now failing validate:
Run ./validate.sh --nofmt --cov --race 10
./validate.sh --nofmt --cov --race 10
shell: /bin/bash -e {0}
env:
GOROOT: /opt/hostedtoolcache/go/1.15.13/x64
GO111MODULE: on
gofmt needs to be run, 1 files have issues. Below is a list of files to review:
adapters/adagio/adagio_test.go
Error: Process completed with exit code 1.

[SyntaxNode]

adagio_test.go

Ah, they just need to run gofmt on this file.

[osazos]

Hi @hhhjort, @SyntaxNode,

I fixed the tests in a proper way, sorry for that.
Let me know if there is something else to improve or fix.

Thx

[SyntaxNode]

Nitpick: You can make the error checking inline:

if _, err := gz.Write(json); err == nil {
  if err := gz.Close(); err == nil {
    json = bodyBuf.Bytes()
    headers.Add("Content-Encoding", "gzip")
    // /!\ Go already sets the `Accept-Encoding: gzip` header. Never add it manually, or Go won't decompress the response.
    //headers.Add("Accept-Encoding", "gzip")
  }
}

[osazos]

Yep, I updated

[AlexBVolcy]

LGTM!

[mansinahar]

The error code in this case should be BadServerResponse as the bidder response doesn't include the bidType. BadInput indicates that the request was malformed or was missing some required info.

[mansinahar]

I know the likelihood of an Unmarshal error here is quite low but still might be a good idea to have an assert.NoError check after the unmarshal rather than skipping the error check completely.

[mansinahar]

Please consider adding a NoError assertion here rather than skipping the error check

[mansinahar]

How is this test case conceptually different from banner-web.json?

[osazos]

This is basically the same, I created this test because the adagio bid extension is not always filled. But you right, I removed this test and moved the extension inside the video-web.json test which is the real case: when we return a video imp, we fill the extension.

[mansinahar]

Please consider renaming this test to missing-ext-bid-type or something along those lines as that's the reason your adapter code is not returning bids

[mansinahar]

Please consider adding JSON tests for status codes other than StatusOK as well in the supplemental directory

[mansinahar]

This looks like a multi-imp test rather than a multiformat test. Please consider renaming it accordingly and adding a multiformat test where a given imp has multiple formats specified for example, both banner and video.

[AlexBVolcy]

I agree with Mansi's comments about a few minor changes if you want to look into those!

[osazos]

Thanks for your deep review @mansinahar, all should be fixed.

[AlexBVolcy]

The updated changes to Mansi's comments look good to me!

[mansinahar]

LGTM! Just minor comments about error variable names

[mansinahar]

Can you please rename this to errs rather? errors is the error handling package in the standard Go library and can therefore be confusing

[mansinahar]

@osazos looks like you missed this one

[osazos]

you right… done :)

[osazos]

@mansinahar, done.
Thanks for your time on this review.

[mansinahar]

@osazos your welcome :) Looks like you missed renaming one of the error variables I pointed out above. Can you please update that? I am happy to approve thereafter. Thanks!

[AlexBVolcy]

Thank you for responding to all of the feedback!