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.

Sign up for GitHub

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

Jump to bottom

Criteo: versioned Fastbid wrapper #6158

Merged
merged 5 commits into from
Mar 15, 2021
Merged

Criteo: versioned Fastbid wrapper #6158

merged 5 commits into from
Mar 15, 2021

Conversation

bmwcmw
Copy link
Collaborator

@bmwcmw bmwcmw commented Jan 4, 2021

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Following last discussions #5804 #5239, we're offering a versioned wrapper of our fastbid script and allow users to specify fastBidVersion pointing to different source files. Therefore, the default version will be specified in the criteo adapter and be updated manually.

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer:
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@mike-chowla mike-chowla requested a review from msm0504 January 6, 2021 01:20
msm0504
msm0504 approved these changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@msm0504 msm0504 left a comment

Choose a reason for hiding this comment

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

@bmwcmw The changes look good to me. Let me know when this can be merged.

@bmwcmw bmwcmw added the Prebid 5.0 Bookmarked for inclusion in breaking version 5.0 label Jan 20, 2021
@bmwcmw
Copy link
Collaborator Author

bmwcmw commented Jan 20, 2021

Hi @msm0504 thanks, it could be part of the 5.0 release. I added a label, should I remove the "do not merge" now or is there anything else to do?

@bmwcmw bmwcmw force-pushed the fastbid_version_wrapper branch 2 times, most recently from 7dceb4f to 313e774 Compare January 27, 2021 16:38
ncolletti
ncolletti reviewed Feb 4, 2021
View reviewed changes
modules/criteoBidAdapter.js Outdated
@@ -17,8 +17,11 @@ const storage = getStorageManager(GVLID);
const LOG_PREFIX = 'Criteo: ';

// Unminified source code can be found in: https://github.com/Prebid-org/prebid-js-external-js-criteo/blob/master/dist/prod.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan to update this to include the un-minified publishertag code? This link is broken and the existing prebid-js-external repo is empty. If there isn't we could remove this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @ncolletti, the link refers to a private repository shared to prebid so needs the access, it's kept here as a hint

@gglas gglas linked an issue Mar 8, 2021 that may be closed by this pull request
Criteo Adapter Changes #5804
Closed
@msm0504 msm0504 changed the base branch from master to prebid-5.0 March 8, 2021 20:18
@msm0504
Copy link
Contributor

msm0504 commented Mar 8, 2021

@bmwcmw There's now a branch for Prebid 5.0 changes. Can this PR be merged into that branch?

@ncolletti Can this PR be approved or are you waiting for more changes to be made?

@bmwcmw
Copy link
Collaborator Author

bmwcmw commented Mar 11, 2021

@bmwcmw There's now a branch for Prebid 5.0 changes. Can this PR be merged into that branch?

@ncolletti Can this PR be approved or are you waiting for more changes to be made?

Hi @msm0504 @ncolletti it's ok for us to merge into 5.0 branch, I just removed the do not merge label.

@bmwcmw bmwcmw force-pushed the fastbid_version_wrapper branch from a9f6517 to 8ca445c Compare March 11, 2021 17:46
@msm0504 msm0504 added the LGTM label Mar 15, 2021
@msm0504 msm0504 merged commit 00f32c9 into prebid:prebid-5.0 Mar 15, 2021
@bmwcmw bmwcmw deleted the fastbid_version_wrapper branch March 17, 2021 14:25
@patmmccann patmmccann mentioned this pull request Mar 23, 2021
Merged
patmmccann added a commit that referenced this pull request Mar 23, 2021
@patmmccann
restore 5.0 branch (#6463)
07742da 
* Criteo: versioned Fastbid wrapper (#6158)

* Support loading specific version of fastbid script

* Return specified current version of Fastbid by default

* Increment adapter version

* Update default fastbid adapter version

* Update default fastbid adapter version

Co-authored-by: mi.chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncolletti ncolletti ncolletti left review comments

@msm0504 msm0504 msm0504 approved these changes

Assignees

@msm0504 msm0504

Labels
LGTM Prebid 5.0 Bookmarked for inclusion in breaking version 5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Criteo Adapter Changes
5 participants
@bmwcmw @msm0504 @ncolletti @ChrisHuie @mike-chowla