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

feat: MakesFederatedCall servant combinator #2

Closed
wants to merge 6 commits into from

Conversation

isovector
Copy link
Owner

Checklist

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

@isovector isovector temporarily deployed to cachix December 20, 2022 23:07 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:08 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:10 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:10 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:19 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:19 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:27 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:27 — with GitHub Actions Inactive
@@ -175,10 +175,10 @@ userAPI =
:<|> deleteLocale
:<|> getDefaultUserLocale

authAPI :: (Member GalleyProvider r, CallsFed 'Brig "on-user-deleted-connections") => ServerT BrigIRoutes.AuthAPI (Handler r)
authAPI :: (Member GalleyProvider r) => ServerT BrigIRoutes.AuthAPI (Handler r)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This PR lets us safely eliminate this CallsFed constraint.

toSwagger (Proxy @api)
& applyTags
[ Tag
"x-wire-makes-federated-call-to"
Copy link

Choose a reason for hiding this comment

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

i just built this branch and tried this out, but came up empty:

$ curl -q http://localhost:8082/api/swagger.json | jq . | grep x-wire-makes-federated-call-to

am i holding it wrong?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not sure how to run the server, but dumping the output of Wire.API.Routes.Internal.Brig.swaggerDoc gives me the following output:

swagger.json.txt

which gives the following output

$ cat /tmp/swagger.json.txt| jq . | grep x-wire-makes-federated-call-to
          "x-wire-makes-federated-call-to"
          "x-wire-makes-federated-call-to"
      "name": "x-wire-makes-federated-call-to"

But upon viewing this in a swagger viewer, it looks like tags might be the wrong place for this information:

2022-12-21-112522_585x281_scrot

@isovector isovector temporarily deployed to cachix December 21, 2022 21:22 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 21:22 — with GitHub Actions Inactive
@isovector
Copy link
Owner Author

Resultant swagger for brig after this change:

swagger.json.txt

@isovector isovector temporarily deployed to cachix December 21, 2022 21:27 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 21:27 — with GitHub Actions Inactive
@fisx
Copy link

fisx commented Dec 22, 2022

could you push this branch to wireapp/wire-server and make a PR from there (either rewire this one or create a new one)? then we can have the CI take a look at things.

it would be interesting to understand where the output you're working with goes, and what goes into http://localhost:8082/api/swagger.json (which i think is the one used here, but i'm ont sure).

@@ -172,6 +172,13 @@ let
sha256 = "1w23yz2iiayniymk7k4g8gww7268187cayw0c8m3bz2hbnvbyfbc";
};
};
swagger2 = {

Choose a reason for hiding this comment

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

Why do we need exactly this version of swagger2?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@mdimjasevic asked the same thing. The swagger specification supports extensions, but swagger2 doesn't. Julia and I decided to patch swagger2 with the functionality and have upstreamed it, but it's unclear if the library is maintained.

@@ -172,6 +172,13 @@ let
sha256 = "1w23yz2iiayniymk7k4g8gww7268187cayw0c8m3bz2hbnvbyfbc";
};
};
swagger2 = {

Choose a reason for hiding this comment

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

Why this Swagger2 pin? I suppose it provides something that currently doesn't exist in the version that is used.

@isovector isovector closed this Dec 22, 2022
@isovector
Copy link
Owner Author

@fisx

could you push this branch to wireapp/wire-server and make a PR from there (either rewire this one or create a new one)? then we can have the CI take a look at things.

will do!

it would be interesting to understand where the output you're working with goes, and what goes into http://localhost:8082/api/swagger.json (which i think is the one used here, but i'm ont sure).

After investigating, this PR currently only changes an internal brig endpoint's definition, and the internal API's swagger isn't served anywhere.

@isovector
Copy link
Owner Author

Reopened at wireapp#2950

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants