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

replace placeholder URLS with commandline variables #86

Closed
wants to merge 8 commits into from

Conversation

OAburub
Copy link

@OAburub OAburub commented Oct 13, 2024

I reported this issue here #85 , this PR should fix it

@Faraz32123
Copy link
Collaborator

Hello @OAburub, can u also create a changelog entry(like this) by using scriv(scriv create).

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.

I don't understand this change. The lms-url and studio-url are supposed to be the internal URL used for communication between containers, right?

@OAburub
Copy link
Author

OAburub commented Oct 15, 2024

@regisb If that is the case, then I severely misunderstood the code, and therefore the error/issue (#86) that made me open this PR.
I assumed that "https://lms:8000" was simply a placeholder that may have been forgotten about, not an internal URL pointing to a container, but it appears that it was being pointed correctly and the error was produced because another part of the platform had not been initialized correctly.

I'm awaiting confirmation on this if possible, then I will close the PR.

@regisb
Copy link
Contributor

regisb commented Oct 15, 2024

No worries. I think you misunderstood the code. The "lms:8000" hostnames are definitely not placeholders. It might be that we need to pass public URLs to the initialization scripts, in which case we would be very wrong, but so far this code has worked, so I'm inclined to think that if there is an issue, then the solution lies elsewhere.

@OAburub
Copy link
Author

OAburub commented Oct 16, 2024

Alright, I will close this and the issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

Successfully merging this pull request may close these issues.

3 participants