You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "martyngigg (via GitHub)" <gi...@apache.org> on 2024/09/01 07:41:47 UTC
[PR] (fix): Move copying translation files before npm run build in Docker [superset]
martyngigg opened a new pull request, #30099:
URL: https://github.com/apache/superset/pull/30099
### SUMMARY
Since #29791 the [webpack.config.js](https://github.com/apache/superset/blob/548d543efe81ecd6f0a6657550230b765ab4d955/superset-frontend/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:
```sh
#19 [superset-node 7/12] RUN npm run build
#19 0.291
#19 0.291 > superset@0.0.0-dev 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:
```sh
docker buildx build --progress plain --target superset-node --build-arg PY_VER=3.10-slim-bookworm --no-cache .
```
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2356376797
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2339068575
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2328431365
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](https://github.com/apache/superset/pull/29791/files#diff-af8c7b9613aada55841f641f037e90dbe5695de25a9240096ef3e3cc907bd021R59). 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2327733772
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #30099:
URL: https://github.com/apache/superset/pull/30099
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2329515543
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2354825379
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2344786775
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2349346098
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!].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2353409470
> 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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2348331313
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](https://github.com/apache/superset/blob/master/.github/workflows/superset-docs-verify.yml#L6) workflow only runs when changes are made to `docs/**` but then the link checker itself also checks things in the [`RELEASING`](https://github.com/apache/superset/blob/master/.github/workflows/superset-docs-verify.yml#L23) 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2357790517
> > 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! 🤦🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Move copying translation files before npm run build in Docker [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30099:
URL: https://github.com/apache/superset/pull/30099#issuecomment-2334645432
I resolved the merge conflict I created in the first place...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org