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

Application service spec cleanup; Security definitions; r0 prep #1555

Merged
merged 7 commits into from
Aug 30, 2018

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Aug 24, 2018

Rendered: see 'docs' status check

This may be best to review commit by commit.


This is a PR for the few small tasks that need to be completed before an r0 can be cut:

  • Support the changelog framework
  • Support the version strings (currently set to unstable)
  • Tidy up empty sections of the RST
  • Add security definitions to the hs->as endpoints

Blocked on:

@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Aug 24, 2018
@@ -254,6 +191,8 @@ paths:
Retreive an array of third party network locations from a Matrix room
Copy link
Member

Choose a reason for hiding this comment

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

Retrieve

@@ -298,6 +237,8 @@ paths:
description: |-
Retreive an array of third party users from a Matrix User ID.
Copy link
Member

Choose a reason for hiding this comment

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

Retrieve

@ara4n
Copy link
Member

ara4n commented Aug 27, 2018

lgtm (modulo feedback in dependent PRs being solved)

turt2live added a commit to turt2live/matrix-doc that referenced this pull request Aug 27, 2018
This commit has approval under matrix-org#1555 although is being included in this branch/PR so the build passes, permitting a merge.
@turt2live turt2live requested a review from a team August 28, 2018 00:05
@turt2live turt2live removed the blocked Something needs to be done before action can be taken on this PR/issue. label Aug 28, 2018
@turt2live turt2live mentioned this pull request Aug 29, 2018
11 tasks
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

A few things I'm unsure about, but it's mostly fine.

changelogs/application_service/pyproject.toml Show resolved Hide resolved
specification/application_service_api.rst Show resolved Hide resolved
@@ -377,8 +392,6 @@ additional parameters on the ``/publicRooms`` client-server endpoint.
Event fields
~~~~~~~~~~~~

.. TODO-TravisR: Fix this section to be a general "3rd party networks" section

We recommend that any events that originated from a remote network should
include an ``external_url`` field in their content to provide a way for Matrix
clients to link into the 'native' client from which the event originated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mention that client's using these should warn about them. From all the other issues raised in the spec, it looks like we are going for cautious?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and do we have a suggested format?

Copy link
Member Author

@turt2live turt2live Aug 30, 2018

Choose a reason for hiding this comment

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

Would be good to clarify this section, yea. Will do that in a dedicated PR.

Edit: #1624

Application Services
--------------------
Application services are passive and can only observe events from a given
homeserver (HS). They can inject events into rooms they are participating in.
Copy link
Contributor

Choose a reason for hiding this comment

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

"from a given homeserver (HS)" I'm worried this might confuse people to believe appservices only work against events originating from one homeserver?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds like we just need to remove "given"?

@turt2live turt2live removed the request for review from a team August 30, 2018 16:16
@turt2live turt2live requested a review from Half-Shot August 30, 2018 16:23
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Just the external_uri and then you can merge :)

@turt2live
Copy link
Member Author

(reminded out of band that this is not an r0, just the prep for it - an r0 is a whole thing)

@turt2live turt2live merged commit 0f2e01f into matrix-org:master Aug 30, 2018
@turt2live turt2live deleted the travis/as/cleanup branch August 30, 2018 17:08
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