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

Fixes #37775 - include hashes in production builds of assets #10125

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Apr 11, 2024

draft, as this only does it for core, not plugins

@@ -153,6 +153,12 @@ const coreConfig = function() {
'webpack/assets/javascripts/bundle.js'
);
config.context = path.resolve(__dirname, '..');
if (config.mode == 'production') {
Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason, this is always true, and I don't understand it.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at

var production =
process.env.RAILS_ENV === 'production' ||
process.env.NODE_ENV === 'production';
const mode = production ? 'production' : 'development';
we very often set NODE_ENV to production.

Copy link
Member Author

Choose a reason for hiding this comment

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

ENV["NODE_ENV"] ||= 'production'

that could be it, indeed

@evgeni
Copy link
Member Author

evgeni commented Apr 11, 2024

Okay, maybe I'm not as smart as I thought…

#<ActionController::RoutingError: No route matches [GET] "/webpack/reactExports.js">

@evgeni
Copy link
Member Author

evgeni commented Apr 11, 2024

I feel like I need webpack_asset_paths, but that was deprecated :/

@evgeni evgeni force-pushed the webpack-hashes branch 3 times, most recently from 478727b to a837946 Compare April 15, 2024 11:30
}
config.output = {
path: outputPath,
publicPath: '/webpack/' + pluginName + '/',
filename: chunkFilename,
Copy link
Member Author

Choose a reason for hiding this comment

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

I am almost sure this will break plugins, but I've not tested it yet

Copy link
Member Author

Choose a reason for hiding this comment

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

amazingly, it does not!

old foreman and old foreman_puppet:
Screenshot from 2024-08-29 14-43-13

new foreman (with hashes) and old foreman_puppet:
Screenshot from 2024-08-29 14-42-44

new foreman (with hashes) and new foreman_puppet (with hashes):
Screenshot from 2024-08-29 16-05-09

@github-actions github-actions bot added the Stale label Jul 25, 2024
Copy link

github-actions bot commented Aug 1, 2024

Thank you for your contribution! This PR has been inactive for 3 months, closing for now. Feel free to reopen when you return to it. This is an automated process.

@github-actions github-actions bot closed this Aug 1, 2024
@ekohl
Copy link
Member

ekohl commented Aug 28, 2024

This is still relevant

@@ -96,7 +96,7 @@ class HostJSTest < IntegrationTestWithJavascript

test "edit page" do
visit host_details_page_path(@host)
click_button 'Edit'
find('.host-details-header-section').find_button('Edit').click # the Edit button for the host details, not the host 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 think this is a regression introduced in 37093a5, but it's weird if we've been ignoring this test failure since May.

Copy link
Member Author

Choose a reason for hiding this comment

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

something, something, doesn't always load quickly enough or whatever.

this test HAS been super flaky, which could be the regression here.

Copy link
Member Author

Choose a reason for hiding this comment

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

#10298

now to find out why the global params one is failing

@ekohl ekohl changed the title include hashes in production builds of assets Fixes #37775 - include hashes in production builds of assets Aug 30, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

👍 once the PR is limited to just the issue. I think we should also pick this to 3.11.

@evgeni
Copy link
Member Author

evgeni commented Aug 30, 2024

3.11 or 3.12?

@ekohl
Copy link
Member

ekohl commented Aug 30, 2024

I think it was introduced as a regression in 89c6104 so:

$ git tag --contains 89c610451c9ea22eb426826e405518e04d68972f | grep -v rc
3.10.0
3.11.0
3.11.1

That means also 3.10.

Copy link
Member

@MariaAga MariaAga left a comment

Choose a reason for hiding this comment

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

Tested it yesterday and I think it did break plugins, I will take a deeper look at it today to make sure

Copy link
Member

@MariaAga MariaAga left a comment

Choose a reason for hiding this comment

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

nvm, tested in dev and prod with foreman ansible and REX and all looks to work as expected!

@ekohl
Copy link
Member

ekohl commented Aug 30, 2024

Should the commit message include a Fixes trailer? So Fixes: 89c610451c9e ("Fixes #37102 - webpack 5")

@evgeni evgeni merged commit cd1ea00 into theforeman:develop Aug 30, 2024
61 checks passed
@evgeni
Copy link
Member Author

evgeni commented Aug 30, 2024

Should the commit message include a Fixes trailer? So Fixes: 89c610451c9e ("Fixes #37102 - webpack 5")

Uh, thanks GitHub for not refreshing the comments, I guess…

It could, but now it does not :)

@evgeni evgeni deleted the webpack-hashes branch August 31, 2024 10:47
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