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

fix: fix base url for the media file links #55

Conversation

Faraz32123
Copy link
Collaborator

Creating this PR for this Issue fix.

@Faraz32123 Faraz32123 self-assigned this Dec 7, 2023
@Faraz32123 Faraz32123 requested a review from regisb December 7, 2023 12:14
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Let's avoid new settings. Also, can you please add a changelog entry? You commit message (NOT title) should also include Close #46 to refer to the issue.

"overrides": {
"DISCOVERY_BASE_URL": PROTOCOL + "://{{DISCOVERY_HOST}}",
"MEDIA_URL": PROTOCOL + "://{{DISCOVERY_HOST}}/media/",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not introduce new Tutor settings here. Instead, the templates should be like:

DISCOVERY_BASE_URL = "{% if ENABLE_HTTPS %}https{% else %}http{% endif %}://{{ DISCOVERY_HOST }}:8381"
MEDIA_URL = DISCOVERY_BASE_URL + "/media"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you are talking about PROTOCOL, It was a variable. I have removed it and used ENABLE_HTTPS now.

close overhangio#46 as this PR will fix base url for discovery and media file links
@Faraz32123 Faraz32123 force-pushed the fix/faraz-fix_base_url_for_media_links branch from 73ca49f to 74f4534 Compare December 8, 2023 12:55
@Faraz32123 Faraz32123 requested a review from regisb December 8, 2023 12:59
@regisb regisb mentioned this pull request Dec 9, 2023
@regisb
Copy link
Contributor

regisb commented Dec 9, 2023

Replaced by #57

@regisb regisb closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants