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

Kargo adapter #1316

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Kargo adapter #1316

merged 1 commit into from
Jul 12, 2017

Conversation

samuelhorwitz
Copy link
Contributor

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
  • Other

Description of change

Adding an adapter for Kargo Global, Inc..

  • test parameters for validating bids
{
  bidder: 'kargo',
  params: {
    placementId: '_lak5xuxuRo'
  }
},
{
  bidder: 'kargo',
  params: {
    placementId: '_m1Xt2E5dez'
  }
}

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

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

Other information

@samuelhorwitz
Copy link
Contributor Author

samuelhorwitz commented Jun 22, 2017

@protonate sorry if you are not the right person to ask here, but the Travis build is failing due to a Criteo test that fails (I haven't touched any of that code) and it also looks like the test suite bails before even finishing all the tests. I'm not sure how to resolve this but I don't believe it's our code that is triggering the Travis failure.
https://travis-ci.org/prebid/Prebid.js/builds/245448854#L1026

@dbemiller
Copy link
Contributor

@samuelhorwitz No worries, you're absolutely right :).

Criteo fixed the issue in #1308. If you pull in the most recent changes those failures should go away.

@samuelhorwitz
Copy link
Contributor Author

Cool thanks! Rebased everything, it's still bailing out at test 443 which might be our end, I will re-test locally again.

@samuelhorwitz
Copy link
Contributor Author

Fixed up my tests and it appears that everything runs all the way through now.

update

safe defaults

removing kargo adapters

adding adapter to list

linting

removing describe.only

fixing up tests

updates
@samuelhorwitz
Copy link
Contributor Author

samuelhorwitz commented Jun 27, 2017

@dbemiller @protonate I have updated this PR to reflect all the re-organization of code on master and all the tests are passing with good (above 80%) coverage results. Is there anything else I need to do for this PR?

@dbemiller
Copy link
Contributor

dbemiller commented Jun 27, 2017

@samuelhorwitz No promises either way yet :).

Time to review this isn't scheduled in our current sprint... so I wouldn't expect anyone to look at it until at least next week. We'll let you know, though.

@samuelhorwitz
Copy link
Contributor Author

Cool, sounds good!

@samuelhorwitz
Copy link
Contributor Author

Hey guys does this week seem likely for a review?

@dbemiller dbemiller added the LGTM label Jul 12, 2017
@dbemiller dbemiller self-assigned this Jul 12, 2017
@dbemiller
Copy link
Contributor

Looks great to me. Very nice code... you should be proud :).

FYI: You might want to update the docs explaining your bidder params too.

@samuelhorwitz
Copy link
Contributor Author

Thanks! I have a documentation update here

prebid/prebid.github.io#274

@matthewlane matthewlane merged commit 4b9136c into prebid:master Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants