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

Server static assets in production based on environment detection #20107

Merged
merged 2 commits into from
May 15, 2020

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Apr 27, 2020

  • Additionally adds tests for a number of environment conditions

@jrafanie please Review
cc @himdel

@@ -24,6 +24,10 @@ def self.supports_nohup_and_backgrounding?
@supports_nohup = is_appliance? && supports_command?('nohup')
end

def self.is_production_build?
Copy link
Member Author

Choose a reason for hiding this comment

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

We can debate the name...I was having a hard time with this.

Note that in discussions with @simaishi, it might be preferable to have a dedicated ENV var for this, such as MANAGEIQ_IMAGE=true, which would be present in all build artifacts whether appliance, podified, or container. Then, this becomes a simple check for that ENV VAR, and all of the other checks in this file can use that value as a an extra sanity check.

Copy link
Member

Choose a reason for hiding this comment

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

If we honored RAILS_SERVE_STATIC_FILES's value, these types of installations can set this env variable and this method is not needed. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

RAILS_SERVE_STATIC_FILES is meant for dev use case of testing locally when Rails.env == "production"...that is, devs are expected to set it. The rationale is that in production Rails static asset serving is off by default, and then you opt-in with the env var, so on real production you specifically don't set anything.

@Fryguy Fryguy force-pushed the production_static_assets branch from 9af6b20 to cbc55d8 Compare May 15, 2020 17:47
@Fryguy
Copy link
Member Author

Fryguy commented May 15, 2020

@jrafanie As discussed I've also added a warning output for the abnormal use cases so users know what is happening and aren't surprised.

@miq-bot
Copy link
Member

miq-bot commented May 15, 2020

Some comments on commits Fryguy/manageiq@40620c9~...cbc55d8

config/environments/production.rb

  • ⚠️ - 18 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented May 15, 2020

Checked commits Fryguy/manageiq@40620c9~...cbc55d8 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
3 files checked, 2 offenses detected

config/environments/production.rb

  • ❗ - Line 18, Col 3 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

lib/miq_environment.rb

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

Awesome. This is great. Knowing that rails is serving static assets in production mode is a nice addition as clearly, that's not a normal thing I do unless I'm doing performance testing of slow requests.

@jrafanie jrafanie merged commit daa6751 into ManageIQ:master May 15, 2020
@Fryguy Fryguy deleted the production_static_assets branch May 15, 2020 19:22
simaishi pushed a commit that referenced this pull request May 21, 2020
Server static assets in production based on environment detection

(cherry picked from commit daa6751)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 4247cbad85fe5cae1e809266f50100a144a15185
Author: Joe Rafaniello <[email protected]>
Date:   Fri May 15 15:14:03 2020 -0400

    Merge pull request #20107 from Fryguy/production_static_assets

    Server static assets in production based on environment detection

    (cherry picked from commit daa6751d737b4546716ae5ed9b547a8b1c727f3b)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants