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

CWire: Add domainId param and app capability #4107

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

CWire: Add domainId param and app capability #4107

wants to merge 5 commits into from

Conversation

roarc0
Copy link

@roarc0 roarc0 commented Dec 16, 2024 •
edited
Loading

This updates the CWire adapter:

  1. adds 'domainId'
  2. adds the 'app' capability.

The docs are already updated since we did the analogous change for Prebid.js.

@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, f45505e

cwire

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:32:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:45:	MakeRequests	87.5%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:72:	MakeBids	100.0%
total:								(statements)	95.7%

bsardo
bsardo reviewed Dec 16, 2024
View reviewed changes
static/bidder-info/cwire.yaml
@@ -5,6 +5,9 @@ endpointCompression: gzip
gvlVendorID: 1081
modifyingVastXmlAllowed: false
capabilities:
site:
app:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an exemplary simple app JSON test.

Copy link
Author

Choose a reason for hiding this comment

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

ok!

Copy link
Author

Choose a reason for hiding this comment

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

I added two JSONs containing OpenRTB app request and also the additional domainId field.

@bsardo bsardo changed the title CWire: update prebid server adapter CWire: Add domainId param and app capability Dec 16, 2024
@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 6fa75b9

cwire

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:32:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:45:	MakeRequests	87.5%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:72:	MakeBids	100.0%
total:								(statements)	95.7%

@bsardo bsardo self-assigned this Dec 16, 2024
@roarc0 roarc0 requested a review from bsardo December 16, 2024 19:30
@przemkaczmarek przemkaczmarek self-assigned this Dec 17, 2024
@roarc0
Copy link
Author

roarc0 commented Dec 18, 2024 •
edited
Loading

validate merge check was failing on:
FAIL github.com/prebid/prebid-server/v3/adapters/pubmatic 0.049s

I rebased it..

@roarc0 roarc0 force-pushed the update-cwire-adapter branch from 6fa75b9 to 61a49fc Compare December 18, 2024 07:58
@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 61a49fc

cwire

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:32:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:45:	MakeRequests	87.5%
github.com/prebid/prebid-server/v3/adapters/cwire/cwire.go:72:	MakeBids	100.0%
total:								(statements)	95.7%

@roarc0
Copy link
Author

roarc0 commented Dec 20, 2024

hello! I don't mean to put any pressure but is there any chance this could be approved? Does it need any refinements? thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@przemkaczmarek przemkaczmarek przemkaczmarek approved these changes

@bsardo bsardo Awaiting requested review from bsardo

At least 2 approving reviews are required to merge this pull request.

Assignees

@bsardo bsardo

@przemkaczmarek przemkaczmarek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@roarc0 @bsardo @przemkaczmarek