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

Vungle: Rename from liftoff #3727

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Vungle: Rename from liftoff #3727

merged 3 commits into from
Jun 11, 2024

Conversation

Vungle-GordonTian
Copy link
Contributor

Copy link

github-actions bot commented Jun 5, 2024

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, 5a8fc9d

vungle

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:34:	MakeRequests	68.2%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:120:	MakeBids	93.3%
total:									(statements)	75.0%

@SyntaxNode
Copy link
Contributor

Please also update adapter_util.go#GetDisabledBidderWarningMessages to add liftoff as a removed adapter. This allows Prebid Server to understand the adapter used to exist and respond to the publisher with a nice message. You can include the rename to vungle in the message, similar to yssp entry.

@SyntaxNode SyntaxNode changed the title rename Adapter from liftoff to vungle vungle: rename from liftoff Jun 5, 2024
@Vungle-GordonTian
Copy link
Contributor Author

Please also update adapter_util.go#GetDisabledBidderWarningMessages to add liftoff as a removed adapter. This allows Prebid Server to understand the adapter used to exist and respond to the publisher with a nice message. You can include the rename to vungle in the message, similar to yssp entry.

Got it. Thx. BTW, what if there is a new adapter named liftoff in the future? Remove this warning?

Copy link

github-actions bot commented Jun 5, 2024

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, c252c33

vungle

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:34:	MakeRequests	68.2%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:120:	MakeBids	93.3%
total:									(statements)	75.0%

@SyntaxNode
Copy link
Contributor

Got it. Thx. BTW, what if there is a new adapter named liftoff in the future? Remove this warning?

Perhaps. Do you intend to add a liftoff adapter in the future?

@Vungle-GordonTian
Copy link
Contributor Author

Got it. Thx. BTW, what if there is a new adapter named liftoff in the future? Remove this warning?

Perhaps. Do you intend to add a liftoff adapter in the future?

No. Just curious. And thx. I've update the adapter_util.go.

@bsardo bsardo self-assigned this Jun 5, 2024
@bsardo bsardo changed the title vungle: rename from liftoff Vungle: Rename from liftoff Jun 5, 2024
SyntaxNode
SyntaxNode previously approved these changes Jun 6, 2024
@SyntaxNode SyntaxNode self-assigned this Jun 6, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename this file to imp_vungle.go.

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
Contributor

Choose a reason for hiding this comment

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

Good catch. I searched the code for liftoff references, but missed the file names. :)

Copy link

github-actions bot commented Jun 6, 2024

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, 37a8fc3

vungle

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:34:	MakeRequests	68.2%
github.com/prebid/prebid-server/v2/adapters/vungle/vungle.go:120:	MakeBids	93.3%
total:									(statements)	75.0%

@bsardo bsardo merged commit 2d2bf71 into prebid:master Jun 11, 2024
5 checks passed
@bretg
Copy link
Contributor

bretg commented Jun 14, 2024

I want to remind key folks about the guidelines around renaming. It's potentially unpleasant to publishers and host companies to pull the rug out from under them by eliminating names suddenly. People might get confused why errors start happening.

Which is why we don't allow abrupt renames like this without a transition period. Liftoff can become an alias of vungle and then at some point in a major release liftoff can be removed

  1. please update the code so the vungle adapter lists liftoff as an alias.
  2. leave liftoff documentation in place, create vungle docs

@PBDMSFT
Copy link

PBDMSFT commented Jun 14, 2024

Hi @bretg I can't speak for the Liftoff/Vungle team, but from the Microsoft Monetize side, this adapter is not live yet. We've been testing together but, to my knowledge, no one is actively using this adapter. The Liftoff/Vungle team chose to rename their adapter and we have already put in the time to mirror that change on the Monetize side. Unless there are actually any publishers using the adapter through another wrapper, we would strongly prefer to use "Vungle" as that's what we just spent time accommodating on our side.

@bretg
Copy link
Contributor

bretg commented Jun 14, 2024

That's good to hear @PBDMSFT , but once something's live in open source, how do we know who's using it?

Unless I get a Vungle/Liftoff person to guarantee they have no customers on 'liftoff', Prebid's policy is that the old name has to stay around for a while to give pubs/host companies time to shift.

@PBDMSFT
Copy link

PBDMSFT commented Jun 14, 2024

Understood. Will watch for them to comment. CC @Vungle-GordonTian

@Vungle-GordonTian
Copy link
Contributor Author

Vungle-GordonTian commented Jun 15, 2024

That's good to hear @PBDMSFT , but once something's live in open source, how do we know who's using it?

Unless I get a Vungle/Liftoff person to guarantee they have no customers on 'liftoff', Prebid's policy is that the old name has to stay around for a while to give pubs/host companies time to shift.

I believe Xandr is the one and only customer that being test with us now, and I will also let our business PM group to confirm this and reach out to you ASSP. @bretg
Many thanks for the guidance. @PBDMSFT

-CC. @yingchen0706v @xiaopeng0216

mefjush pushed a commit to adhese/prebid-server that referenced this pull request Jul 19, 2024
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