fix: Move copying translation files before npm run build in Docker

[martyngigg]

SUMMARY

Since #29791 the webpack.config.js file now requires that the translation files are present when running the build. This change simply moves the copy of the translation files before the npm run build step.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Beforehand, an error printed was printed, although it doesn't seem to stop the build, that can cause confusion as to whether something is wrong with the build:

#19 [superset-node  7/12] RUN npm run build
#19 0.291
#19 0.291 > [email protected] build
#19 0.291 > cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production BABEL_ENV="${BABEL_ENV:=production}" webpack --color --mode production
#19 0.291
#19 1.087 Error reading the directory: Error: ENOENT: no such file or directory, scandir '/app/superset/translations'
#19 1.087     at Object.readdirSync (node:fs:1507:26)
#19 1.087     at getAvailableTranslationCodes (/app/superset-frontend/webpack.config.js:52:22)
#19 1.087     at Object.<anonymous> (/app/superset-frontend/webpack.config.js:168:20)
#19 1.087     at Module._compile (node:internal/modules/cjs/loader:1469:14)
#19 1.087     at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
#19 1.087     at Module.load (node:internal/modules/cjs/loader:1288:32)
#19 1.087     at Module._load (node:internal/modules/cjs/loader:1104:12)
#19 1.087     at Module.require (node:internal/modules/cjs/loader:1311:19)
#19 1.087     at require (node:internal/modules/helpers:179:18)
#19 1.087     at WebpackCLI.tryRequireThenImport (/app/superset-frontend/node_modules/webpack-cli/lib/webpack-cli.js:204:22) {
#19 1.087   errno: -2,
#19 1.087   code: 'ENOENT',
#19 1.087   syscall: 'scandir',
#19 1.087   path: '/app/superset/translations'
#19 1.087 }

TESTING INSTRUCTIONS

Build the superset-node stage and ensure it completes without errors relating to the translations files:

docker buildx build --progress plain --target superset-node --build-arg PY_VER=3.10-slim-bookworm --no-cache .

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[mistercrunch]

Oh this will impact the docker-cache hit-rate a bit. If anything in the translation folder changes we won't be able to reuse the frontend build cache, which is probably ok and required anyways.

I have to admit I didn't know if the translation was more of a build-time thing or a run-time thing. Given the new logic in the webpack config it appears it's required at build time. Hoping it doesn't impact perf when using en, please confirm if you can.

We may want to introduce a new ARG build_translations=false to speed up build and reduce image size for the large majority on a en-only deploy.

[martyngigg]

Thanks for the review!

I have to admit I didn't know if the translation was more of a build-time thing or a run-time thing. Given the new logic in the webpack config it appears it's required at build time. Hoping it doesn't impact perf when using en, please confirm if you can.

We may want to introduce a new ARG build_translations=false to speed up build and reduce image size for the large majority on a en-only deploy.

I think this would cause more data to be loaded when the moment-timezone library is lazy loaded by the time zone selector. Before my changes the docker image would only keep the en timezone data but now it would include everything for the locales defined in the translations directory - I'm not sure on the exact size of this but it is only loaded when the timezone selector is made visible on the alerts/reports modal.

Is the preference still to add the build flag and then also remove the translations from the backend and have the standard image be en only?

[mistercrunch]

Took the time to understand the new optimization in webpack.config.js, it's a build time optimization specific to moment's (time processing library) i18n. I looks in the folder for language subfolders and do it based on that. Presumalby moment's support many more language than Superset does (about 20) so that trims the excess.

Note that we have a LANGUAGES config that is used by the admin to set which languages are made available in their environment, ideally that optimization would be in-line with that list instead of looking at subfolders in the repo. It can be a bit tricky to share config between the backend and frontend, currently the main mechanism for this is the backend sends a payload "bootstrap_data" encoded in the html of the page sent to start the "single-page-app", but that doesn't work for build time. We would need to have something like languages.json that both the backend's config and the frontend webpack.confic.js could read from.

Overall unclear how much this matters (could be good to know the bundle size without any locale, with the 20 we support, and with all locales). I vote to keep things simple and simply adding a setting ARG build_translations=false (or similar) and allow to skip some of the complexity and weight for those who use en only.

[mistercrunch]

I resolved the merge conflict I created in the first place...

[martyngigg]

I seem to have a couple of failed tests and I can't quite see why.

• The docs build has detected some broken links - maybe these are really expired?
• One of the matrix values for the E2E tests has failed with a crashed Chrome but I can't see why what I have done here to have broken that?

Thanks!

[mistercrunch]

The docs build has detected some broken links - maybe these are really expired?

rebase and should be fine

random fail in matrix

unfortunately we have some flakiness in these tests, should re-run after you rebase+push

[martyngigg]

I think I've hit a bad link in the release notes that wasn't picked up when the change was merged to master?

The Docs Testing/Link Checking workflow only runs when changes are made to docs/** but then the link checker itself also checks things in the RELEASING directory so it wasn't picked up.

Shall I fix it here or do you have a policy that that should be fixed in a separate PR?

[mistercrunch]

Oh, thinking prior to this PR the effective default is BUILD_TRANSACTION=true so this is kind of a breaking change for those who build/ship translations. We could either set the default to true here or add an entry in UPDATING.md ("if your environment requires translations, you'll want to set the docker build arg BUILD_TRANSACTION=true").

If we decide to switch the default to true, I would set the default in docker-compose.* to false as most development environments don't need translations.

Given the semi-unknown/uneven state of translations coverage and quality, I vote for setting the default to false (as it is now), but that means this PR needs to add an entry in UPDATING.md as suggested above.

[mistercrunch]

I'm not sure why @github-actions E2E / cypress-matrix (0, chrome) (pull_request) check is failing, at first I thought it was test flakiness, but I manually restarted a few times and seems to consistently fail. Restarting it once again.

For the docs, I think the failed check is non-blocking for merging, and there's new commits in master making those just warning, so rebasing once again might help here [sorry about the confusion!].

[rusackas]

Restarting it once again.
All is passing!

For the docs, I think the failed check is non-blocking for merging
Please re-review/unblock the PR if you think it's OK to go, so either of us can merge it after giving @martyngigg a little extra time for touchups :)

[martyngigg]

Oh, thinking prior to this PR the effective default is BUILD_TRANSACTION=true so this is kind of a breaking change for those who build/ship translations. We could either set the default to true here or add an entry in UPDATING.md ("if your environment requires translations, you'll want to set the docker build arg BUILD_TRANSACTION=true").

If we decide to switch the default to true, I would set the default in docker-compose.* to false as most development environments don't need translations.

Given the semi-unknown/uneven state of translations coverage and quality, I vote for setting the default to false (as it is now), but that means this PR needs to add an entry in UPDATING.md as suggested above.

Thanks for merging this :)

Sorry for not responding to the above comment sooner but I didn't have time to look at this yesterday. As I didn't have time to add the entry in UPDATING.md shall I open a new PR for that?

[mistercrunch]

shall I open a new PR for that?

Oh I forgot about my own comment, a new comment in UPDATING.md would be nice if you don't mind opening a one-liner

[martyngigg]

shall I open a new PR for that?

Oh I forgot about my own comment, a new comment in UPDATING.md would be nice if you don't mind opening a one-liner

No problem :)

Ignore the first closed PR above that I managed to open on my fork by accident! 🤦🏻