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

Don't recommend :8448 to people on public_baseurl #4498

Merged
merged 2 commits into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@turt2live
Copy link
Member

turt2live commented Jan 28, 2019

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off (N/A)

turt2live added some commits Jan 28, 2019

@turt2live turt2live requested a review from matrix-org/synapse-core Jan 28, 2019

@aaronraimist

This comment has been minimized.

Copy link
Contributor

aaronraimist commented Jan 28, 2019

Also here:

# public_baseurl: https://example.com:8448/

I guess it's not in the docker config?

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Jan 28, 2019

That file doesn't exist on develop, so I assume it is being auto-generated now.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 28, 2019

Codecov Report

Merging #4498 into develop will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4498      +/-   ##
===========================================
+ Coverage    74.73%   74.74%   +<.01%     
===========================================
  Files          336      336              
  Lines        34148    34148              
  Branches      5553     5553              
===========================================
+ Hits         25520    25523       +3     
- Misses        7049     7050       +1     
+ Partials      1579     1575       -4
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jan 29, 2019

looks plausible, but assumes people have somehow forwarded port 443 to synapse. Was the existing config causing confusion?

@richvdh richvdh removed the request for review from matrix-org/synapse-core Jan 29, 2019

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Jan 29, 2019

Yes, people keep breaking their riots because this endpoint causes a redirect. Even with the additional documentation, people keep sticking :8448 on the end because they think it is required.

@richvdh
Copy link
Member

richvdh left a comment

I'm unconvinced that this is going to be any better, but whatever.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jan 29, 2019

[also, once we require people to have proper certs on port 8448, does the problem go away?]

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Jan 29, 2019

[also, once we require people to have proper certs on port 8448, does the problem go away?]

Probably, assuming people have client listeners.

@turt2live turt2live merged commit d02c5cc into develop Jan 29, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turt2live turt2live deleted the travis/fix-docs-public_baseurl branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment