Skip to content

move integrety check config to yaml #1669

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

Merged

Conversation

lsylvester
Copy link
Contributor

Currently the webpacker:install task updates the development.rb and production.rb environment files.

This is problematic in rails/rails#33079 as it causes some of the tests to hang while waiting for the user to confirm if they want to overwrite the files.

This PR moves this configuration into the webpacker.yml by default.

Setting it in environment files is still supported for backwards compatibility. I am not sure if this should be deprecated, or expanded to allow for anything to be set via the Rails.application.config.

In order to be able to merge the configs set in the environment files and those loaded in the yml file, I have moved the reloading of the config out of the bootstrapping, and added the webpacker.yml to the watch so that spring restarts the environment when it changes.

@danielpowell4
Copy link

Why would you ever want to set the default of this to something other true? Seems like setting to false is promoting bad practices

@lsylvester
Copy link
Contributor Author

@danielpowell4 Are you referring to having being false in non-development environments or having the ability to change in for development?

In production like environments the check isn't required as the assets are already precompiled.

In development, we are defaulting to having it turned on, so that is the practice we are promoting.

The default configurations aren't changing as a result of this, just how it is configured.

@gauravtiwari gauravtiwari merged commit c2d032c into rails:master Aug 28, 2018
@gauravtiwari
Copy link
Member

thanks @lsylvester

@danielpowell4 On production this check doesn't really matter since it's not possible to alter the dependencies in any way (precompiled) compared to development.

@danielpowell4
Copy link

I was thinking that failing precompilation, which uses a potentially outdated frozen lockfile, could also be a safe default.

What you all did also makes perfect sense to me though!

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

Successfully merging this pull request may close these issues.

3 participants