Skip to content

Serialisation of client capabilities (take 2)#3909

Merged
pcapriotti merged 10 commits intodevelopfrom
pcapriotti/capabilities
Apr 5, 2024
Merged

Serialisation of client capabilities (take 2)#3909
pcapriotti merged 10 commits intodevelopfrom
pcapriotti/capabilities

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Feb 28, 2024

Fix an oddity in the serialisation of capabilities in the Client structure.

This PR replaces #3873 and simplifies the implementation. No new types are added, and the difference in serialisation is obtained using the Versioned wrapper, which we are already using for versioned changes in serialisation.

The change in the internal API between brig and galley is not backward compatible, so this can cause temporary failures during upgrades. The invariant that brig's clients are a superset of galley's clients will be maintained.

Note: #3904 is a previous attempt at this fix, which overlooked the fact that the Client structure is used in user events, so it was reverted.

https://wearezeta.atlassian.net/browse/WPB-6400

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 28, 2024
@pcapriotti pcapriotti force-pushed the pcapriotti/capabilities branch from 36c12a7 to 997b33a Compare March 6, 2024 14:25
@pcapriotti pcapriotti force-pushed the pcapriotti/capabilities branch from 997b33a to 997dcc2 Compare March 18, 2024 14:59
@pcapriotti pcapriotti marked this pull request as ready for review March 18, 2024 15:05
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a big deal, ignore this if you like. but in general (a) i don't like single-character identifiers, even locally; (b) i want us all to avoid edit wars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't like single-character identifiers, but two-character ones are ok? That's a bit of a strange stance. But sure, I'll revert it.

@mdimjasevic mdimjasevic force-pushed the pcapriotti/capabilities branch from ff26b74 to 1ce2ed8 Compare April 4, 2024 13:16
@mdimjasevic
Copy link
Contributor

mdimjasevic commented Apr 4, 2024

@pcapriotti , in an attempt to get the PR through the CI, I took the liberty to rebase the PR on top of develop. I hope you don't mind.

@mdimjasevic
Copy link
Contributor

As far as I can tell, now we're getting actual test failures: https://concourse.ops.zinfra.io/builds/49403780

@pcapriotti
Copy link
Contributor Author

As far as I can tell, now we're getting actual test failures: https://concourse.ops.zinfra.io/builds/49403780

Thanks for rebasing. Yes, that's right. It should be fixed now.

@pcapriotti pcapriotti merged commit 5589540 into develop Apr 5, 2024
@pcapriotti pcapriotti deleted the pcapriotti/capabilities branch April 5, 2024 08:16
@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 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