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

New Adapter: VideoHeroes #2399

Merged
merged 7 commits into from
Jan 4, 2023
Merged

Conversation

videoheroes
Copy link
Contributor

No description provided.

@bretg
Copy link
Contributor

bretg commented Oct 6, 2022

docs PR prebid/prebid.github.io#4040

}

func TestInvalidParams(t *testing.T) {
validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we confirm that this file path will always be correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is correct. Please refer to https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html paragraph "Bidder Parameter Tests"
This is a path to {bidder}.json, it's always in this directory related to adapter code.

return mediaType
}
}
return mediaType
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you cover this line of code with tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this case should not happen. It indicates that imp id from response is not found in imp[] from request.
In code many adapters do the same. I think this is ok, but we can discuss it with the team.

Comment on lines +5 to +14
app:
mediaTypes:
- banner
- video
- native
site:
mediaTypes:
- banner
- video
- native
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you support these media types, but dont see this reflected in your docs PR. Could you update it?

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Oct 12, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 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 submitting your PR. Added minor comments.

adapters/videoheroes/videoheroes.go Outdated Show resolved Hide resolved
@AlexBVolcy
Copy link
Contributor

There is a PR #2370 that is being merged today that will cause conflicts to new adapter PRs.

To resolve these conflicts, you'll need to do the following:

In videoheroes.go, update the Builder() function signature to include a new parameter server config.Server

In videoheroes_test.go, update any test that includes a call to Builder() to pass a config.Server object, to that call (i.e. config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "datacenter"})

After the merge, you can look at other adapters to see how the server object is implemented as an example.

@SyntaxNode
Copy link
Contributor

This code is identical to #2396. If these are aliases of the same underlying bidder, it's better to implement it just once and use an alias. Less code to maintain.

@SyntaxNode SyntaxNode mentioned this pull request Oct 12, 2022
@videoheroes
Copy link
Contributor Author

This code is identical to #2396. If these are aliases of the same underlying bidder, it's better to implement it just once and use an alias. Less code to maintain.

It can be the same adapters code (one development), but there different projects and will be different farther

@videoheroes
Copy link
Contributor Author

There is a PR #2370 that is being merged today that will cause conflicts to new adapter PRs.

To resolve these conflicts, you'll need to do the following:

In videoheroes.go, update the Builder() function signature to include a new parameter server config.Server

In videoheroes_test.go, update any test that includes a call to Builder() to pass a config.Server object, to that call (i.e. config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "datacenter"})

After the merge, you can look at other adapters to see how the server object is implemented as an example.

added commit with updates

@videoheroes
Copy link
Contributor Author

Please check it after updates ;)

@bsardo
Copy link
Collaborator

bsardo commented Nov 7, 2022

Hi @videoheroes , can you merge with master when you get a chance? Your code is failing the go 1.19 validation checks due to go fmt changes that were introduced with the recent upgrade to go 1.19 on master.

Also your code is failing the validate-merge step due to some recent changes we made on master regarding the ORTB library. You'll need to update the reference to the mxmcherry version of the library to the prebid version.

@videoheroes
Copy link
Contributor Author

Hi guys, just merged with last version of master and pushed it again, could you please check it ?

@bsardo
Copy link
Collaborator

bsardo commented Nov 15, 2022

@videoheroes you'll need to update the reference to the mxmcherry library in your adapter to github.com/prebid/openrtb/v17/openrtb2. We now use the prebid version of the openrtb2 library.

@videoheroes
Copy link
Contributor Author

heya Guys, could you please review the last updates ?

}

func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
var videoHeoresExt *openrtb_ext.ExtImpVideoHeroes
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in videoHeoresExt -> videoHeroesExt

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll approve once this typo is fixed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

AlexBVolcy
AlexBVolcy previously approved these changes Dec 6, 2022
@bsardo
Copy link
Collaborator

bsardo commented Dec 14, 2022

Hi @videoheroes, can you please undo any changes made to go.mod and go.sum before we merge? Perhaps running go mod tidy will help clear that up.

@videoheroes
Copy link
Contributor Author

Hi @bsardo, I just saw your message and downgraded version of mod, sum files to "162aa91 Update Go version to 1.19 (#2428)"

@videoheroes videoheroes requested review from VeronikaSolovei9 and AlexBVolcy and removed request for VeronikaSolovei9 and AlexBVolcy December 28, 2022 13:58
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

.

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for removing changes to go.mod and go.sum

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo merged commit fa50f2e into prebid:master Jan 4, 2023
aidarbek pushed a commit to aidarbek/prebid-server that referenced this pull request Jan 12, 2023
shahinrahbariasl pushed a commit to indexexchange/prebid-server that referenced this pull request Jan 18, 2023
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jan 31, 2023
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.

6 participants