Adapter aliases: moved adapter related configs to bidder.yaml files #2353

VeronikaSolovei9 · 2022-08-18T03:12:22Z

all adapter related configs are moved to static/bidder-info/{bidder}.yaml files.
Apapter struct now carries only adapter needed data
all adapter related configs are removed from config.go
bidder list now loads from the file system

Docs update PR: prebid/prebid.github.io#4008

# Conflicts: # config/config.go

config/bidderinfo.go

AlexBVolcy · 2022-08-24T20:47:14Z

config/bidderinfo.go

-func (s *SyncerEndpoint) Override(original *SyncerEndpoint) *SyncerEndpoint {
-	if s == nil && original == nil {
-		return nil
+func ProcessBidderInfos(path string, confBidderInfos BidderInfos) (BidderInfos, []error) {


Nitpick: Could you re-name confBidderInfos to configBidderInfos

AlexBVolcy · 2022-08-24T20:49:19Z

config/bidderinfo.go

+
+	for _, bidderConfig := range bidderConfigs {
+		if bidderConfig.IsDir() {
+			continue //or throw an error?


Have you decided if you are going to not throw an error here? If so, could you remove the comment?

AlexBVolcy · 2022-08-24T20:50:05Z

config/bidderinfo.go

+// ToGVLVendorIDMap transforms a BidderInfos object to a map of bidder names to GVL id. Disabled
+// bidders are omitted from the result.
+func (infos BidderInfos) ToGVLVendorIDMap() map[openrtb_ext.BidderName]uint16 {
+	m := make(map[openrtb_ext.BidderName]uint16, len(infos))


Nitpick: Is there a more descriptive name we can use for this variable other than m. I understand if you'd prefer to keep it this way.

This is moved from another place, I didn't change content. Renamed for future convenience.

AlexBVolcy · 2022-08-24T20:50:27Z

config/bidderinfo.go


-	if s.RedirectURL != "" {
-		copy.RedirectURL = s.RedirectURL
+// vlidateBidderInfos validates bidder endpoint, info and syncer data


Nitpick: Spelling error in comment (vlidate)

config/bidderinfo.go

AlexBVolcy · 2022-08-24T21:05:02Z

Also just wanted to comment highlighting the merge conflicts with bidderinfo., bidderinfo_test.go, adapter_util_test.go

# Conflicts: # config/bidderinfo.go # config/bidderinfo_test.go # exchange/adapter_util_test.go

config/bidderinfo.go

config/config.go

config/config_test.go

AlexBVolcy

Very minor comments

AlexBVolcy · 2022-08-31T15:40:57Z

config/bidderinfo.go

+		}
+
+		bidderName := strings.Split(fileName, ".yaml")
+		infos[(bidderName[0])] = info


Is it guaranteed that there will be data present at index 0 here? Just want to check

good point. Added check if file is a yaml file and array len.

AlexBVolcy · 2022-08-31T15:50:00Z

config/bidderinfo.go

+	for _, v := range bidderInfo.Syncer.Supports {
+		if !strings.EqualFold(v, "iframe") && !strings.EqualFold(v, "redirect") {
+			return fmt.Errorf("syncer could not be created, invalid supported endpoint: %s", v)


Could this variable v be more descriptive? I understand if you want to leave it the way it is, but I tend to like a little more from variable names

# Conflicts: # config/config.go # static/bidder-info/yssp.yaml

config/config.go

AlexBVolcy · 2022-09-06T16:33:59Z

This looks good! I want to confirm that exchange about bidderInfos and viper is resolved before approving!

# Conflicts: # static/bidder-info/adkernel.yaml # static/bidder-info/invibes.yaml # static/bidder-info/triplelift.yaml

hhhjort

LGTM

SyntaxNode · 2022-09-12T18:23:18Z

config/bidderinfo.go

-		infos[bidder] = info
+func validatePlatformInfo(info *PlatformInfo) error {
+	if len(info.MediaTypes) == 0 {
+		return errors.New("mediaTypes should be an array with at least one string element")


Nitpick: There is no need to include the wording "should be an array". Of course it's an array. Consider changing to an error along the line of "at least one meda type needs to be specified".

SyntaxNode · 2022-09-12T18:32:19Z

config/bidderinfo.go

 }

-type infoReaderFromDisk struct {
-	path string
+func applyBidderInfoConfigOverrides(configBidderInfos BidderInfos, fsBidderInfos BidderInfos) (BidderInfos, error) {


This is no longer necessary. We can let Viper take care of this work. You need to only give Viper the bidder infos before calling unmarshal and it will take care of the overrides:

func New(v *viper.Viper, bidderInfos BidderInfos) (*Configuration, error) { var c Configuration c.BidderInfos = bidderInfos if err := v.Unmarshal(&c); err != nil { return nil, fmt.Errorf("viper failed to unmarshal app config: %v", err) } ...

This breaks is the legacy user sync url, which we can either choose to remove as part of this work or we alert applyBidderInfoConfigOverrides to only apply the legacy user sync url override.

This also removes our ability to disallow certain parts of the config to be changed, like the maintainer or media types. The later can cause a problem if it's less restrictive.

I think we can either:

Let Viper handle everything and remove the override code. This allows hosts to "shoot themselves in the foot", but there's nothing stop them from editing the bidder info files directly?

Keep control of overrides and create a new BidderInfoOverride with less fields and leave the override code.

Thoughts?

This is what you showed me on our very first discussion about the approach to support env variables.
We also had a discussion about it with Hans: #2353 (comment)

If I set c.BidderInfos = bidderInfos and have env variable for at least one field for a bidder (for example endpoint) - it overrides the entire object for this bidder and sets "endpoint" only. Other data from yaml file is not present for this bidder after config umnarshalling.

I think we need to control overrides. I removed capabilities from overrides, we can discuss what other fields should be removed. But I also think this is not obvious why some of the fields can be overridden and some not. Please advice.

SyntaxNode · 2022-09-12T18:41:05Z

config/config.go

+	v.BindEnv(adapterCfgPrefix+".xapi.username", "")
+	v.BindEnv(adapterCfgPrefix+".xapi.password", "")
+	v.BindEnv(adapterCfgPrefix+".xapi.tracker", "")
+	v.BindEnv(adapterCfgPrefix+".endpointCompression", "")


There be in danger in allowing unrestricted overrides of capabilities.app.mediaTypes and capabilities.site.mediaTypes.

removed support of the capabilities override

SyntaxNode · 2022-09-12T18:42:03Z

config/config_test.go

+
+	assert.Equal(t, "platform_id_override", cfg.BidderInfos["bidder1"].PlatformID)
+	assert.Equal(t, "app_secret_override", cfg.BidderInfos["bidder1"].AppSecret)
+	assert.Equal(t, "GZIP", cfg.BidderInfos["bidder1"].EndpointCompression)
 }


IMHO we don't need to test all of the env variables, just a few is fine to provide viper binding works ok. This seems a little too verbose IMHO.

Removed some of checks

SyntaxNode · 2022-09-12T18:42:38Z

config/test/bidder-info-invalid/someBidder-invalid.yaml

@@ -0,0 +1,3 @@
+maintainer:
+  email: 123
+gvlVendorID: "42"


Why do we need do add this? Unit tests should cover this already.

removed. Just wanted to test it with actual yaml file.

SyntaxNode · 2022-09-12T18:43:28Z

endpoints/events/vtrack.go

@@ -267,7 +267,7 @@ func isAllowVastForBidder(bidder string, bidderInfos *config.BidderInfos, allowU
 	// check if bidder is configured
 	if b, ok := (*bidderInfos)[bidder]; bidderInfos != nil && ok {
 		// check if bidder is enabled
-		return b.Enabled && b.ModifyingVastXmlAllowed
+		return !b.Disabled && b.ModifyingVastXmlAllowed


I recommend adding a helper method to bidder info so we can call .IsEnabled() instead of negating the disabled field. The later is harder to read and easier to miss.

Agree, modified

# Conflicts: # endpoints/openrtb2/test_utils.go

hhhjort · 2022-09-14T18:44:10Z

config/bidderinfo.go

@@ -315,7 +319,7 @@ func validateCapabilities(info *CapabilitiesInfo, bidderName string) error {

 func validatePlatformInfo(info *PlatformInfo) error {
 	if len(info.MediaTypes) == 0 {
-		return errors.New("mediaTypes should be an array with at least one string element")
+		return errors.New("at least one meda type needs to be specified")


media has a typo 'meda'

# Conflicts: # config/config_test.go

AlexBVolcy

LGTM

* origin/master: (316 commits) Fix alt bidder code test broken by imp wrapper commit (prebid#2405) ImpWrapper (prebid#2375) Update sspBC adapter to v5.7 (prebid#2394) Change errors to warnings (prebid#2385) TheMediaGrid: support native mediaType (prebid#2377) Huaweiadsadapter gdpr (prebid#2345) Add alternatebiddercodes in Prebid request ext (prebid#2339) Changed FS bidder names to mixed case (prebid#2390) Load bidders names from file system in lowercase (prebid#2388) consumable adapter: add schain and coppa support (prebid#2361) Adapter aliases: moved adapter related configs to bidder.yaml files (prebid#2353) pubmatic adapter: add param prebiddealpriority (prebid#2281) Yieldlab Adapter: Add support for ad unit sizes (prebid#2364) GDPR: add enforce_algo config flag and convert enforce_purpose to bool (prebid#2330) Resolves CVE-2022-27664 (prebid#2376) AMP Endpoint: support parameters consentType and gdprApplies (prebid#2338) Dianomi - added usync for redirect (prebid#2368) Create issue_prioritization.yml (prebid#2372) Update yaml files for gzip supported bidders (prebid#2365) Adnuntius Adapter: Set no cookies as a parameter (prebid#2352) ...

…rebid#2353) * Adapter aliases: moved adapter related configs to bidder.yaml files * Moved adapter configs from config.go to bidder.yaml files * Removed ymal mappers * Clean up * Initial unit tests fix * Merge with latest master * Unit tests clean up * Added pbs.json or pbs.yaml config support * Fixes and unit tests, added ssyncer merge functions back * Merge fixes * Returned back adapter config set up using env variables * Unit tests fixes * Added test for adapter config overrides from environment variables * Minor improvements * Added more tests fro env variables, fixed mapstructures from bidder info * Code review refactoring * Code review refactoring and merge conflicts fixes * Typo fix Co-authored-by: vsolovei <[email protected]>

Adapter aliases: moved adapter related configs to bidder.yaml files

5263f0d

VeronikaSolovei9 marked this pull request as draft August 18, 2022 03:12

vsolovei added 8 commits August 19, 2022 17:40

Moved adapter configs from config.go to bidder.yaml files

f2a838c

Removed ymal mappers

4cb1eca

Clean up

23a2aae

Initial unit tests fix

ab39537

Merge branch 'master' into adapter_aliases_config

4b65efb

# Conflicts: # config/config.go

Merge with latest master

4beadbe

Unit tests clean up

c75c1ba

Added pbs.json or pbs.yaml config support

628740e

VeronikaSolovei9 marked this pull request as ready for review August 24, 2022 06:41

VeronikaSolovei9 commented Aug 24, 2022

View reviewed changes

config/bidderinfo.go Show resolved Hide resolved

bsardo self-requested a review August 24, 2022 17:47

bsardo self-assigned this Aug 24, 2022

bsardo requested a review from hhhjort August 24, 2022 17:47

bsardo assigned hhhjort Aug 24, 2022

bsardo requested review from AlexBVolcy and removed request for bsardo August 24, 2022 17:48

bsardo assigned AlexBVolcy and unassigned bsardo Aug 24, 2022

AlexBVolcy reviewed Aug 24, 2022

View reviewed changes

vsolovei added 3 commits August 24, 2022 19:59

Fixes and unit tests, added ssyncer merge functions back

60969cd

Merge branch 'master' into adapter_aliases_config

ac90716

# Conflicts: # config/bidderinfo.go # config/bidderinfo_test.go # exchange/adapter_util_test.go

Merge fixes

655ae7b

hhhjort reviewed Aug 26, 2022

View reviewed changes

config/bidderinfo.go Show resolved Hide resolved

hhhjort reviewed Aug 26, 2022

View reviewed changes

config/config.go Show resolved Hide resolved

hhhjort reviewed Aug 26, 2022

View reviewed changes

config/config_test.go Show resolved Hide resolved

vsolovei added 2 commits August 29, 2022 19:44

Returned back adapter config set up using env variables

6800d8a

Unit tests fixes

a514378

Added test for adapter config overrides from environment variables

712f29d

AlexBVolcy reviewed Aug 31, 2022

View reviewed changes

vsolovei added 2 commits August 31, 2022 11:53

Minor improvements

9f88c59

Merge branch 'master' into adapter_aliases_config

c7673a7

# Conflicts: # config/config.go # static/bidder-info/yssp.yaml

hhhjort reviewed Sep 2, 2022

View reviewed changes

config/config.go Show resolved Hide resolved

Merge branch 'master' into adapter_aliases_config

1649624

# Conflicts: # static/bidder-info/adkernel.yaml # static/bidder-info/invibes.yaml # static/bidder-info/triplelift.yaml

VeronikaSolovei9 mentioned this pull request Sep 8, 2022

Adapter configuration update prebid/prebid.github.io#4008

Merged

Added more tests fro env variables, fixed mapstructures from bidder info

cf35d4d

hhhjort previously approved these changes Sep 9, 2022

View reviewed changes

AlexBVolcy previously approved these changes Sep 12, 2022

View reviewed changes

SyntaxNode reviewed Sep 12, 2022

View reviewed changes

Code review refactoring

13c2142

VeronikaSolovei9 dismissed stale reviews from AlexBVolcy and hhhjort via 13c2142 September 13, 2022 05:58

vsolovei added 2 commits September 13, 2022 14:39

Merge branch 'master' into adapter_aliases_config

c2c8b9d

# Conflicts: # endpoints/openrtb2/test_utils.go

Code review refactoring and merge conflicts fixes

66e8575

hhhjort reviewed Sep 14, 2022

View reviewed changes

Merge branch 'master' into adapter_aliases_config

b0da512

# Conflicts: # config/config_test.go

AlexBVolcy previously approved these changes Sep 15, 2022

View reviewed changes

Typo fix

d3ae21e

VeronikaSolovei9 dismissed AlexBVolcy’s stale review via d3ae21e September 15, 2022 23:28

AlexBVolcy approved these changes Sep 16, 2022

View reviewed changes

hhhjort approved these changes Sep 20, 2022

View reviewed changes

hhhjort merged commit fb564dd into master Sep 20, 2022

SyntaxNode deleted the adapter_aliases_config branch March 13, 2023 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter aliases: moved adapter related configs to bidder.yaml files #2353

Adapter aliases: moved adapter related configs to bidder.yaml files #2353

VeronikaSolovei9 commented Aug 18, 2022 •

edited

Loading

AlexBVolcy Aug 24, 2022

AlexBVolcy Aug 24, 2022

AlexBVolcy Aug 24, 2022

VeronikaSolovei9 Aug 25, 2022

AlexBVolcy Aug 24, 2022

AlexBVolcy commented Aug 24, 2022

AlexBVolcy left a comment

AlexBVolcy Aug 31, 2022

VeronikaSolovei9 Aug 31, 2022

AlexBVolcy Aug 31, 2022

VeronikaSolovei9 Aug 31, 2022

AlexBVolcy commented Sep 6, 2022

hhhjort left a comment

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

SyntaxNode Sep 12, 2022

VeronikaSolovei9 Sep 13, 2022

hhhjort Sep 14, 2022

AlexBVolcy left a comment

Adapter aliases: moved adapter related configs to bidder.yaml files #2353

Adapter aliases: moved adapter related configs to bidder.yaml files #2353

Conversation

VeronikaSolovei9 commented Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy commented Aug 24, 2022

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy commented Sep 6, 2022

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Aug 18, 2022 •

edited

Loading