Skip to content

FS-1059 Change commit-bundle body type to protobuf format#2773

Merged
elland merged 16 commits intodevelopfrom
FS-1059-protobuf-commitbundle
Oct 24, 2022
Merged

FS-1059 Change commit-bundle body type to protobuf format#2773
elland merged 16 commits intodevelopfrom
FS-1059-protobuf-commitbundle

Conversation

@smatting
Copy link
Contributor

@smatting smatting commented Oct 19, 2022

This PR changes the API of the POST /mls/commit-bundles:

This PR also adds missing tests for the endpoint and removes a superfluous test

Tracked in https://wearezeta.atlassian.net/browse/FS-1059

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@smatting smatting temporarily deployed to cachix October 19, 2022 11:56 Inactive
@smatting smatting force-pushed the FS-1059-protobuf-commitbundle branch from dff1fa6 to ce31357 Compare October 19, 2022 12:03
@smatting smatting temporarily deployed to cachix October 19, 2022 12:03 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 19, 2022
@smatting smatting temporarily deployed to cachix October 19, 2022 13:01 Inactive
@smatting smatting force-pushed the FS-1059-protobuf-commitbundle branch from fa91041 to 703e81b Compare October 20, 2022 12:25
@smatting smatting temporarily deployed to cachix October 20, 2022 12:25 Inactive
@smatting smatting force-pushed the FS-1059-protobuf-commitbundle branch from 703e81b to 1c20fd6 Compare October 20, 2022 12:29
@smatting smatting temporarily deployed to cachix October 20, 2022 12:29 Inactive
@smatting smatting marked this pull request as ready for review October 20, 2022 12:29
@smatting smatting temporarily deployed to cachix October 20, 2022 12:34 Inactive
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

Looks good. Reusing the existing code in Wire.API.ServantProto might be nice, but this is fine too. Regardless, I would maybe change the mime type to application/x-protobuf for consistency with the other endpoints.

mimeUnrenderMLSWith :: Get a -> LByteString -> Either String a
mimeUnrenderMLSWith p = first T.unpack . decodeMLSWith p

data CommitBundleMimeType
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use Proto from Wire.API.ServantProto here, instead.

testCase "validate message signature" testVerifyMLSPlainTextWithKey,
testCase "create signed remove proposal" testRemoveProposalMessageSignature,
testCase "parse GroupInfoBundle" testParseGroupInfoBundle
testCase "parse GroupInfoBundle" testParseGroupInfoBundle -- TODO: remove this also
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover TODO?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since @smatting is not here to clarify, I'm unsure whether to make this futurework or disable this test. It is currently green for me anyway…

@elland elland temporarily deployed to cachix October 24, 2022 08:42 Inactive
@elland elland temporarily deployed to cachix October 24, 2022 08:50 Inactive
@elland elland temporarily deployed to cachix October 24, 2022 09:28 Inactive
@elland elland temporarily deployed to cachix October 24, 2022 11:42 Inactive
@elland elland merged commit 8d0c53a into develop Oct 24, 2022
@elland elland deleted the FS-1059-protobuf-commitbundle branch October 24, 2022 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments