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

Do not fallback-compile missing assets #17741

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

skateman
Copy link
Member

All our assets are served through the asset pipeline so this should be disabled.

@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @himdel
@miq-bot add_label enhancement

Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM, better catch these error sooner than later 👍

(Not sure about the implications for people doing development in production mode though.)

@skateman
Copy link
Member Author

Development in production mode sounds wrong 😆 🤣

@martinpovolny martinpovolny self-assigned this Jul 23, 2018
@martinpovolny
Copy link
Member

@jrafanie, @Fryguy : any idea who might have objections?

If not I'd merge this.

@@ -19,8 +19,7 @@
config.assets.compress = true

# Fallback to the asset pipeline if a precompiled asset is missed.
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment incorrect now? We're either going to serve a precompiled asset or it's going to raise an error, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, should I just drop it? It's a default Rails comment 😉

Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling we changed it... the default is now:

  # Do not fallback to assets pipeline if a precompiled asset is missed.
  config.assets.compile = false

We're sneaky...

from here

Copy link
Member

Choose a reason for hiding this comment

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

🙏 git blame doesn't say I did it

@@ -19,8 +19,7 @@
config.assets.compress = true

# Fallback to the asset pipeline if a precompiled asset is missed.
# TODO: Once everything is in the asset pipeline, this should be set back to false.
config.assets.compile = true
config.assets.compile = false
Copy link
Member

Choose a reason for hiding this comment

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

🎉 Yay! 🌮

This always felt weird to maintain two types of assets, ones served through the pipeline and others that were not.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we're serving assets through apache and conf.public_file_server.enabled is default to false.

@jrafanie
Copy link
Member

@bdunne and @simaishi should know about this... I'm guessing the main change is we'll now get errors if assets can't be found instead of just-in-time-compile of it.

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

Checked commit skateman@ec5601f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@martinpovolny
Copy link
Member

@jrafanie, @bdunne and @simaishi whose turn is now?

@jrafanie
Copy link
Member

jrafanie commented Aug 7, 2018

Sorry, I didn't realize this was sitting for 2 weeks. I'd like @simaishi to confirm this is ok since I have very little confidence in what this would mean for the build process or live loading of assets in cases where the asset wasn't properly included. We can merge for now if it's blocking you but I'd rather wait for the people who know the process to approve of it.

If we fail to include an asset due to a mistake or a build problems, am I correct in assuming this will now create errors where previously it would have silently compiled the asset on the fly?

@simaishi
Copy link
Contributor

simaishi commented Aug 7, 2018

I don't see any issue with the change

@bdunne bdunne merged commit 8a79376 into ManageIQ:master Aug 7, 2018
@bdunne bdunne added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 7, 2018
@bdunne bdunne assigned bdunne and unassigned martinpovolny Aug 7, 2018
@skateman skateman deleted the asset-compile branch August 8, 2018 09:06
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.

7 participants