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

Drop signatures for public key fields where not needed. #187

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

SanjayVas
Copy link
Member

This is a breaking API change, but attempts to maintain wire-compatibility for EDP Requisition fulfillment.

@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas force-pushed the sanjayvas-public-key-sig branch from c1ecea1 to e3e7f27 Compare October 24, 2023 23:12
@SanjayVas SanjayVas marked this pull request as ready for review October 31, 2023 22:47
@SanjayVas
Copy link
Member Author

This is a follow-up to #185.

Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 78 at r1 (raw file):

      // (--
      // TODO(world-federation-of-advertisers/cross-media-measurement-api#188):
      // Remove this once the certificate is always specified in

you can remove this if you wait for PR 171 to merge first. wdyt?


src/main/proto/wfa/measurement/api/v2alpha/requisition.proto line 84 at r1 (raw file):

  // Remove this once the certificate is always specified in
  // `FulfillDirectRequisitionRequest`. --)
  string data_provider_certificate = 6 [

ditto

Base automatically changed from sanjayvas-type-urls to main November 1, 2023 16:56
@SanjayVas SanjayVas force-pushed the sanjayvas-public-key-sig branch from e3e7f27 to 92e2912 Compare November 1, 2023 17:00
Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 78 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you can remove this if you wait for PR 171 to merge first. wdyt?

Not quite. The always part here is important, meaning all EDPs need to be specifying the new field.


src/main/proto/wfa/measurement/api/v2alpha/requisition.proto line 84 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

ditto

Ditto.

Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas force-pushed the sanjayvas-public-key-sig branch from 92e2912 to 9ae62d6 Compare November 13, 2023 20:35
This is a breaking API change, but attempts to maintain wire-compatibility for EDP Requisition fulfillment.
@SanjayVas SanjayVas force-pushed the sanjayvas-public-key-sig branch from 9ae62d6 to ecdc4b1 Compare November 13, 2023 20:37
Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

@SanjayVas SanjayVas merged commit 5a7f1e8 into main Nov 13, 2023
3 checks passed
@SanjayVas SanjayVas deleted the sanjayvas-public-key-sig branch November 13, 2023 20:39
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.

3 participants