Add app_url to the app's config and use it for OpenAI images #532
Conversation
|
One thing I don't quite understand is how will this change once the Rails PRs get merged in which you referenced in this comment: #424 (comment) Will we end up removing these environment variables or changing how the rails config works? |
| def not_finished? | ||
| !finished? | ||
| end | ||
| def not_finished? = !finished? |
There was a problem hiding this comment.
I really like this new feature of ruby
There was a problem hiding this comment.
yeah definitely nice to have a shorthand. reminds me of Elixir
| @@ -37,6 +37,14 @@ class Application < Rails::Application | |||
| config.time_zone = "Central Time (US & Canada)" | |||
| config.eager_load_paths << Rails.root.join("lib") | |||
|
|
|||
| if Setting.validate_env_vars == "1" | |||
| Setting.require_keys!(:app_url_protocol, :app_url_host, :app_url_port) | |||
| end | |||
There was a problem hiding this comment.
I don't love this extra flag (validate_env_vars) which changes whether the keys are required or not. What would it take for us to always make the keys optional? For example, maybe we do this:
- If any of these vars are set then we require that all 3 are set. But you can leave all 3 blank and the app still works.
- Would this mean that then within
open_ai.rbwe just add a conditional so that it falls back to file_data_url() if the url_* details are not provided. - It would be nice to have a conditional only in a single place so can we avoid also adding a conditional inside
_message.html? Maybe you makedocument_image_urlbe smart and it simply return the path if the url_* details are unknown?
There was a problem hiding this comment.
yeah it's a bit convoluted as is. I just did that optional piece to avoid it validating during the step to precompile assets. I mainly wanted to have this validation to make the app "fail fast" if the necessary values to create the url weren't present. If we fall back to the path or the base64 path then it should be ok since it still works with base64 urls. I'll update this branch
config/environments/production.rb
Outdated
| @@ -98,6 +98,7 @@ | |||
| # /.*\.example\.com/ # Allow requests from subdomains like `www.example.com` | |||
| # ] | |||
| config.hosts = ENV["PRODUCTION_HOST"].split(",").map(&:strip) if ENV["PRODUCTION_HOST"] | |||
| # config.hosts << Setting.app_url_host | |||
There was a problem hiding this comment.
Why does development add this to config.hosts but production not add it?
There was a problem hiding this comment.
ah I meant to uncomment this one. Where is PRODUCTION_HOST being created? Maybe it would be better to combine them? Should probably do the same with the DEV_HOST env too?
docker-compose.yml
Outdated
| - APP_URL_PROTOCOL | ||
| - APP_URL_HOST | ||
| - APP_URL_PORT | ||
| - VALIDATE_ENV_VARS |
There was a problem hiding this comment.
I was going to say that we need to search the code base for RUN_SOLID_QUEUE_IN_PUMA to find all the other places we're handling env config for our production environments, but we won't need to do that if we can make all the keys successfully be optional.
| test "document_image_path with fallback" do | ||
| assert_equal "", messages(:examine_this).document_image_path(:small, fallback: "") | ||
| test "document_image_url with fallback" do | ||
| assert_equal "", messages(:examine_this).document_image_url(:small, fallback: "") |
There was a problem hiding this comment.
If we can make the keys optional, then can we easily test both configurations with automated tests? I think so by using our stubbing technique for options.yml. But if that is tricky for some reason so that we can only have automated tests for one approach, then I think we should have the automated tests for the default config (i.e. all url_* being blank)
krschacht
changed the title
If they make the changes then we should be able to remove this code: https://github.com/AllYourBot/hostedgpt/pull/532/files#diff-f9c61ca57681712e397b045ba90ceee0d6f073f15b3c1266fa781e6634460d05R155-R159 and then yeah the config will need to change to use the new rails url instead of our custom one and we can remove our env vars too |
|
getting a lot of "Test is missing assertions" warnings during tests. will add a commit to fix those |
@jlvallelonga I went ahead and revert main so we unbreak new users deploying to Render and Heroku. This PR just tee's up the same changes that were in the other PR so we can go back and forth on any specifics.