[online] Revert GENERATOR_HOST explicit setting #3287
Conversation
|
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
| ENV GENERATOR_HOST=http://localhost | ||
| # GENERATOR_HOST can be used to determine the target location of a download link. | ||
| # The default value asumes binding to host via: docker -p 8080:8080 image_name | ||
| ENV GENERATOR_HOST=http://localhost:8080 |
There was a problem hiding this comment.
I'm not sure I understand. Do we keep GENERATOR_HOST?
There was a problem hiding this comment.
@cbornet GENERATOR_HOST is still used here:
This isn't "correct" and will need to be changed. The previous value here would be incorrect for all invocations unless bound to port 80, which I believe requires super user privileges. This change at least causes the variable to match the exposed port.
Many people try out docker images by binding to random host ports (-P), and when doing this… GENERATOR_HOST will be useless and the file download link generation won't work.
This PR limits focus to get the proxying host setting working on the server. I can look at the other work later.
There was a problem hiding this comment.
Created #3288 to address this other concern.
|
CircleCI failure is unrelated. Merging this to let the deployment do its thing, and creating an issue for tracking removal of |
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.master,4.1.x,5.0.x. Default:master.Description of the PR
Removing
GENERATOR_HOSTusage, and moving configuration to server. We may want to consider removingGENERATOR_HOSTcompletely from GenApiServer (if possible), and just using built-in support for retrieving the host/port. I was trying to reduce confusion around this variable in the code that is removed by this PR, but upon discussion it seems the environment variable just adds confusion and is therefore not the appropriate configuration to target.