Skip to content

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jan 28, 2022

This moves internal endpoints to the Internal module hierarchy in wire-api, so that they are not included in the generated Swagger, and removes usages of servant-generic from Galley's internal API.

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.
  • 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.

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.

I suspect this will conflict with #2059. Not sure what to do about it, though.

@@ -731,59 +729,64 @@ type FeatureAPI =
:<|> FeatureConfigGet 'WithLockStatus 'TeamFeatureSelfDeletingMessages
:<|> FeatureConfigGet 'WithLockStatus 'TeamFeatureGuestLinks

type FeatureStatusGet featureName =
type FeatureStatusGet f =
Copy link
Contributor

Choose a reason for hiding this comment

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

please, no one-letter names :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are you picking on this particular one? The codebase is full of one-letter names. Even for this very purpose (see e.g. Galley.API.Teams.Features). I'm actually not sure what the problem is with one-letter names when they stand for very abstract things in a very limited scope (e.g. a single expression, like in this example). Also, the name that was there before isn't even correctly describing what the variable stands for (it's not a "feature name").

@pcapriotti pcapriotti force-pushed the pcapriotti/move-internal-endpoints branch from 434ef39 to 3700c3f Compare February 1, 2022 09:44
@pcapriotti pcapriotti force-pushed the pcapriotti/move-internal-endpoints branch from 3700c3f to f7bea4c Compare February 2, 2022 06:54
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Overall this looks good.

I'm just curious: what is the motivation for removing internal endpoints from Swagger? Should we maybe put it into a different root/folder, but still have Swagger for internal endpoints?

@pcapriotti
Copy link
Contributor Author

Overall this looks good.

I'm just curious: what is the motivation for removing internal endpoints from Swagger? Should we maybe put it into a different root/folder, but still have Swagger for internal endpoints?

Because the current swagger is supposed to present the API as visible from the outside (e.g. the Z-User header is not documented directly). Also, internal endpoints can overlap, and are only meaningful for a given service. For example, it would be nice to see a bunch of /i/status endpoints with no indication of which service they belong to.

@pcapriotti pcapriotti merged commit e16ad2c into develop Feb 4, 2022
@pcapriotti pcapriotti deleted the pcapriotti/move-internal-endpoints branch February 4, 2022 08:18
@fisx fisx mentioned this pull request Feb 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.

3 participants