-
Notifications
You must be signed in to change notification settings - Fork 769
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
Ads.Cert Call Sign support #2241
Merged
Merged
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
1312664
Initial preparation for Ads.Cert Call Sign support
1722b81
First initial working version
384bb22
Added request option and bidder config to enable or disable ads certs
a94d723
Code clean up and README file added
1547a0b
Code refactoring and clean up
7f91db7
Unit tests fix
3e6f780
Merge branch 'master' into ads_cert
82715a9
Merge fixes
9c28dd6
Code refactoring and clean up
0bc246d
Added more go docs for newly defines structures
aea4f71
Added unit tests
9bffd99
Added unit tests
b8baa68
Added unit tests
ac47bd5
Minor clean up
c78b16d
Added remote signer.
540c9f8
Removed optional code
26dac9b
Added tests for NewAdCertsSigner
de1d8fa
Unit tests for remote signer
4b9e937
CR fixes part I
7b6ffe9
CR fixes part II
8991f4e
Code clean up
6257437
Merge branch 'master' into ads_cert
6f87497
Merge fixes
a091df9
Merge branch 'master' into ads_cert
1e012b0
renamed "in-process" to "inprocess" to avoid env variables conflict
8b3a795
Renamed adscert to adsCert
4462439
Code refactoring
8b0617b
Code refactoring and signers config validation
f0ac967
Code refactoring
bb4c9d9
Code refactoring
cea7973
Merge branch 'master' into ads_cert
1636dd3
Resolved merge errors
2d29975
Merge branch 'master' into ads_cert
37d2529
Merge with master
2178fc6
Code review comments
8709b76
Merge branch 'master' into ads_cert
72cae9b
Merge conflict fix
7486973
Moved validation to config.go
a1177a9
Minor fixes
d3d50bb
Modified config from experiment.adscert.enabled to experiment.adscert…
a92f723
Merge branch 'master' into ads_cert
999e6da
Code review fixes
d0a9966
Code review fixes
b9fc196
Code review fixes
5b81715
Code cleanup
c3c0fc2
Code cleanup
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Code refactoring
- Loading branch information
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add appropriate error handling for when error is not
nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea here is to record this to log or metrics. This will be added in separate PR. I don't think we want to discard the entire request if header was not generated properly. Added a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a comment. Are there any requirements to add a warning in response when debug is enabled to let the publisher know that there was a problem signing the request? If so, is the plan to add that as part of a later PR as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a warning back to the publisher/caller is a good idea. I think a separate PR would be appropriate. This is getting close to the end of review and is already large enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR sounds good to me 👍 @VeronikaSolovei9 would you mind just modifying the comment to add a warning message in debug mode as well in addition to the metrics please? Just so that we don't forget about it