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

docs: tweaks install instructions re: port #573

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

debuggingfuture
Copy link

@debuggingfuture debuggingfuture commented Mar 1, 2021

Hi first of all thanks for this great life-saver project!

I installed this to my server recently, some feedback regarding the docs

my setup env:

  • terraform to provision homeserver/this bridge
  • docker-compose on ec2
  • db: rds

feedback re: port

  • I spent sometime to understand the port to be used. It is more obvious when I read the log of runnable process and check the source code. According to
    Consistently suggest to use port 5858 for the appservice #436 the default port is 5858, shall we provide in the instruction as well? (as changed in this PR)

  • I understand it is the port homeserver will connect to, while the name MATRIX_PORT sounds a port homeserver is listening to. Potentially HOMESERVER_PORT SYNAPSE_PORT are more specific?

  • By "specify the direct link", and generating the config with docker, it might not be too obvious that if refers to the "url field" in the config yml when I decided to modify directly. (added a note in this PR)

  • changed the file name as slack-registration.yaml is the recommended name, not slack.yaml

re: /link

  • Probably there are other issues pointing to this already. I was confused link to be a /link command and struggled when it didnt show up. There are nice feedback on the correctness of commands once the appservice is installed, so we can probably hint user to look for this to hint whether it is problem of appservice process or problem of access configurations.

re:slack token (link_channels.md)

  • Seems there are some changes on slack side, so if we "Select bot token scopes", it will not work with below warning
    The RTM API is not accessible to updated Bot Tokens. If your app requires RTM functionality, please exit this update and continue using your existing scopes.
  • instead I chose First, add a legacy bot user to make it work, without further changing its scopes
  • I think screenshots could greatly help, I can create some in another PR

some thoughts for terraform/docker-compose

  • I found out https://github.com/spantaleev/matrix-docker-ansible-deploy only after I have done the deployment. Shall we add a link to that repo?

  • Will a sample terraform/ docker-compose sample of interest? There are some networking tweaks e.g. listen to "http://appservice:5858" instead of localhost, tweaking the volumes mount point etc

Signed-off-by: Vincent Lau lauchunyin@debuggingfuture.com

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.

This looks fine, be sure to add a changelog entry here

docs/getting_started.md Outdated Show resolved Hide resolved
docs/getting_started.md Outdated Show resolved Hide resolved
@Half-Shot
Copy link
Contributor

ping @vincentlaucy

jaller94 and others added 4 commits February 15, 2022 00:05
Co-authored-by: Will Hunt <will@half-shot.uk>
Co-authored-by: Will Hunt <will@half-shot.uk>
docs/getting_started.md Outdated Show resolved Hide resolved
@jaller94
Copy link
Contributor

@Half-Shot Do we need to insist on the Sign-Off?

@vincentlaucy To ensure all code in this project has the same license, please modify the following line and add it to the PR description:
Signed-off-by: Your Name <your@email.example.org>
More info: https://github.com/matrix-org/synapse/blob/v1.4.0/CONTRIBUTING.rst#sign-off

@Half-Shot
Copy link
Contributor

@Half-Shot Do we need to insist on the Sign-Off?

@vincentlaucy To ensure all code in this project has the same license, please modify the following line and add it to the PR description: Signed-off-by: Your Name <your@email.example.org> More info: https://github.com/matrix-org/synapse/blob/v1.4.0/CONTRIBUTING.rst#sign-off

Yep, we do!

Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Sign-Off is missing.

@debuggingfuture
Copy link
Author

Thanks appended the issue with sign off

@AndrewFerr AndrewFerr requested a review from a team as a code owner November 15, 2022 07:41
@zhubonan
Copy link

Hi it would be great to have this merged. I have been following the current docs and using docker to setup but it did not work out right, mainly due to the port configuration and the default name of the registration file.

I managed to solve the issues eventually but then I found the changes I needed are all in this PR. 😂

The other thing I found is that to use a custom port (other than 5858) the docker has to run with the -p option explicitly, just like the system installed approach.

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.

5 participants