Skip to content

Comments

Servantify cargohold and add federationDomain option#1990

Merged
pcapriotti merged 26 commits intodevelopfrom
pcapriotti/cargohold-servant
Dec 20, 2021
Merged

Servantify cargohold and add federationDomain option#1990
pcapriotti merged 26 commits intodevelopfrom
pcapriotti/cargohold-servant

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Dec 13, 2021

This converts the public API to Servant, removes the dependency on wai-routing, and also introduces a mandatory federationDomain configuration option.

Tracked by https://wearezeta.atlassian.net/browse/SQCORE-1174.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If public end-points have been changed or added: does nginz need un upgrade?
    • If internal end-points have been added or changed: which services have to be deployed in a specific order?

@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-servant branch from 96b01b8 to 627c643 Compare December 15, 2021 17:58
@pcapriotti pcapriotti marked this pull request as ready for review December 16, 2021 08:40
@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-servant branch from f34ebcf to 7d40012 Compare December 16, 2021 08:48
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Approving the federationDomain setting option in its various incantations. The rest I just skimmed.

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Could you rebase this so it doesn't contain other PRs any more?

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't match the title of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but it would be somewhat painful to split this into two PR, since the ZAuth servant combinators have integration with the Qualified stuff that needs knowledge of the federation domain, which we'll need in cargohold later anyway. It would just be more pointless work to do this in two steps.

Can I just expand the PR description to include this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the title and description. Ok to merge?

@pcapriotti pcapriotti force-pushed the pcapriotti/cargohold-servant branch from 678ade3 to 64e0bf7 Compare December 17, 2021 13:32
@pcapriotti
Copy link
Contributor Author

Could you rebase this so it doesn't contain other PRs any more?

Done. The unrelated schema changes are now gone.

@pcapriotti pcapriotti changed the title Servantify cargohold Servantify cargohold and add federationDomain option Dec 17, 2021
@pcapriotti pcapriotti merged commit e1866b3 into develop Dec 20, 2021
@pcapriotti pcapriotti deleted the pcapriotti/cargohold-servant branch December 20, 2021 09:19
@akshaymankar akshaymankar mentioned this pull request Jan 18, 2022
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.

4 participants