fix(#29965): add assets prefix on all manifest assets

[huandu]

SUMMARY

Per #29965, this PR fixes the issue that static assets prefix is not added to static assets listed in manifest.json.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• [x] Has associated issue: #29965
• [ ] 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

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (76d897e) to head (118a061). Report is 1787 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/extensions/__init__.py	83.33%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29966       +/-   ##
===========================================
+ Coverage   60.48%   83.64%   +23.15%     
===========================================
  Files        1931      528     -1403     
  Lines       76236    38151    -38085     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31911    -14203     
+ Misses      28017     6240    -21777     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.95% <83.33%> (-0.20%)	:arrow_down:
javascript	?
mysql	76.66% <83.33%> (?)
postgres	76.73% <83.33%> (?)
presto	53.50% <83.33%> (-0.30%)	:arrow_down:
python	83.64% <83.33%> (+20.13%)	:arrow_up:
sqlite	76.22% <83.33%> (?)
unit	60.31% <83.33%> (+2.68%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rusackas]

Thanks for the contribution! You need to get the pre-commit check to pass. Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

[mistercrunch]

You need to get the pre-commit check to pass.

We could add a message in the logs https://github.com/apache/superset/blob/master/.github/workflows/pre-commit.yml#L43-L47, let me try to write a PR that adds a note so new contributors can find the error/solution as they click the failing check

[mistercrunch]

@rusackas @kgabryje , wondering how this work on our infra at preset.io, how did we know to fetch the bundles from CDN? Or did do we fetch bundles from the webserver? Could this break our build?

[mistercrunch]

Here, this should help: https://github.com/apache/superset/pull/29970

[huandu]

@rusackas Thanks for your guidance. I've updated the PR and passed all pre-commit checks.

[huandu]

Actually let's make sure we won't break things for people, or if we do we should add a line item to UDPATING.md as in "if you use the STATIC_ASSETS_PREFIX config, bundles will now be fetched from that location"

I can add this notice in UPDATING.md. I'm not sure which section it should be added to, 4.0.0 or Next or others?

BTW, from my perspective, when I read the comment for STATIC_ASSETS_PREFIX, my understanding is that this config can affect all static assets, including all JS and CSS files used by Superset website. So I think this PR is a bug fix rather than a feature change. What do you think?

https://github.com/apache/superset/blob/cf083bf827e477987d6534f57b6f9dbd4b8ab22a/superset/config.py#L1631-L1633

[rusackas]

@huandu 4.0.x is already out, so I believe you'd put it in the Next section. @sadpandajoe may or may not want to pull this in for 4.1 ¯\_(ツ)_/¯

[eschutho]

@huandu, this could probably be better documented, but can you try to set an environment variable ASSET_BASE_URL to match your STATIC_ASSETS_PREFIX config and see if your js assets build with the correct filepath. I believe this is the way that they are currently being built. For reference: https://github.com/apache/superset/pull/15105

Here's an example from my local testing where STATIC_ASSETS_PREFIX is set to "foo" and ASSET_BASE_URL is set to "myurl:

[huandu]

can you try to set an environment variable ASSET_BASE_URL to match your STATIC_ASSETS_PREFIX config and see if your js assets build with the correct filepath.

@eschutho I tried ASSET_BASE_URL but it doesn't work for me.

From my understanding, this variable only affects Webpack building process. It does nothing when I use the official superset docker image to run a pod in a k8s cluster.

[rusackas]

This looks like it's in need of a rebase, and sadly the conversation got stalled. Is there still interest in getting this mergeable/merged?

[huandu]

Is there still interest in getting this mergeable/merged?

@rusackas I'm OK to close it since it doesn't seem to be an issue concerned by many users. For me, I patched the code in docker image and worked fine for month in my production environment.

[adamfarmer0]

This error still exists in v5.0.0. I had to patch the code with this part:

        loaded_chunks = set()
        assets_prefix = self.app.config["STATIC_ASSETS_PREFIX"] if self.app else ""

        def get_files(bundle: str, asset_type: str = "js") -> list[str]:
            files = self.get_manifest_files(bundle, asset_type)
            filtered_files = [f for f in files if f not in loaded_chunks]
            for f in filtered_files:
                loaded_chunks.add(f)
            if assets_prefix:
                return [f"{assets_prefix}{f}" for f in filtered_files]
            return filtered_files

        return {
            "js_manifest": lambda bundle: get_files(bundle, "js"),
            "css_manifest": lambda bundle: get_files(bundle, "css"),
            "assets_prefix": assets_prefix,
        }

to make it work.