Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add a clarification for the public_baseurl property. #7906

Closed
wants to merge 5 commits into from
Closed

Add a clarification for the public_baseurl property. #7906

wants to merge 5 commits into from

Conversation

zblesk
Copy link

@zblesk zblesk commented Jul 19, 2020

I have been a bit confused about the correct way to set the public_baseurl and did it wrong. I've realized the mistake with some help from the folks in #synapse:matrix.org and they suggested I try and add a clarification to the sample file. So that's what I'm trying to do.

Signed-off-by: Ladislav Benc ladislav.benc@protonmail.com

@clokep
Copy link
Member

clokep commented Jul 20, 2020

@zblesk Can you make the corresponding change in synapse/config/server.py to make CI happy? Thanks!

@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 20, 2020
docs/sample_config.yaml Outdated Show resolved Hide resolved
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
@zblesk
Copy link
Author

zblesk commented Jul 20, 2020

Well. I tried updating until it stopped complainng about mismatches and trailing spaces; now it says No new newsfragments found on this branch. and I have no idea what that means, sorry.

@clokep
Copy link
Member

clokep commented Jul 20, 2020

@zblesk Please take a look at the contributing guide: https://github.com/matrix-org/synapse/blob/develop/CONTRIBUTING.md#changelog

@clokep clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 24, 2020
@clokep clokep requested a review from a team July 24, 2020 20:31
@richvdh richvdh changed the title Adding a clarification for the public_baseurl property. Add a clarification for the public_baseurl property. Jul 26, 2020
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thank you for contributing!

Honestly though, I'm not sure this adds any clarity to what's there already. Doesn't it just repeat the existing description?

existing:

this should be the URL to reach synapse [via the proxy]

new:

this URL should point to Synapse itself.

... those are basically the same, no?

I realise it's confusing, but I don't think that saying the same thing over and over is going to help.

Unfortunately, I don't really have any better suggestions here :/

@richvdh richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 28, 2020
@zblesk
Copy link
Author

zblesk commented Jul 31, 2020

As mentioned above, I'm only doing this because gus in the #synapse channel asked me to, after I got stuck on this point. I do not have any improvements for this.
If it doesn't fit, feel free to close this PR.

@clokep
Copy link
Member

clokep commented Sep 18, 2020

Thanks for your contribution. I'm going to close this per @richvdh's comment above.

@clokep clokep closed this Sep 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants