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

Consumable Adapter - Rewrite to support full ORTB from GO #2927

Merged
merged 9 commits into from
May 22, 2024

Conversation

SuprPhatAnon
Copy link
Contributor

#2865

Full rewrite of Adapter and to match the version in Go

@VeryExtraordinaryUsername
Copy link
Contributor

@SuprPhatAnon do you have anything else to add in this PR, or you are done here?

@SuprPhatAnon
Copy link
Contributor Author

@VeryExtraordinaryUsername This PR is complete

@AntoxaAntoxic
Copy link
Collaborator

Hey @SuprPhatAnon, may I ask you to resolve the conflicts?

@SuprPhatAnon
Copy link
Contributor Author

@AntoxaAntoxic Thank you for the feedback. I can work on getting these changes made. May I ask what changed since this was previously approved and just need some conflicts resolved before?

@AntoxaAntoxic
Copy link
Collaborator

AntoxaAntoxic commented Apr 4, 2024

@SuprPhatAnon Usually a PR is required to be reviewed twice. In other words, approved by at least two people. This is default practice.

@SuprPhatAnon
Copy link
Contributor Author

@AntoxaAntoxic All changes have been made. Waiting for your approval

public ConsumableBidder(String endpointUrl, JacksonMapper mapper) {
this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl));
this.endpointUrl = endpointUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe url validation still makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@SuprPhatAnon
Copy link
Contributor Author

@AntoxaAntoxic Excellent! Thanks! what is the next stop to getting this merged in

@AntoxaAntoxic
Copy link
Collaborator

AntoxaAntoxic commented Apr 25, 2024

It's a merge-freeze stage now before the next major release.

@VeryExtraordinaryUsername VeryExtraordinaryUsername removed their request for review April 25, 2024 14:08
@SuprPhatAnon
Copy link
Contributor Author

This update was a simple rebase from the current master branch

@travisbeale
Copy link

Is there anything else that we need to do in order for this to be merged and included in the next release? We were approved but not merged into the recent 3.0.0 release. Thank you.

@SerhiiNahornyi
Copy link
Collaborator

SerhiiNahornyi commented May 22, 2024

@travisbeale
We tried not to include major bidder changes inside 3.0.

But this PR will be part of the upcoming 3.1 release

@SerhiiNahornyi SerhiiNahornyi merged commit 610e724 into prebid:master May 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants