Skip to content

Conversation

@d3flex
Copy link
Contributor

@d3flex d3flex commented Oct 15, 2025

Replace the `url_for with helper functions which return the domain name whenever this is applicable based on the setting.

  • Link to domain for download assets like iso and Uploaded logs (includes also vars.json)
  • No change in the redirection, assuming that this is out of scope here, and that in general it stays despite the direct link in the href.

issue: https://progress.opensuse.org/issues/189888

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Looks good so far :-)

@d3flex d3flex force-pushed the feat/asset_link_to_subdomain branch from fdc58bb to 3351dd2 Compare October 16, 2025 18:40
@codecov
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.26%. Comparing base (a802d1d) to head (86a6c47).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6804   +/-   ##
=======================================
  Coverage   99.25%   99.26%           
=======================================
  Files         402      402           
  Lines       41363    41385   +22     
=======================================
+ Hits        41056    41079   +23     
+ Misses        307      306    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

https://progress.opensuse.org/issues/189888 says "to simplify our implementation" but you add a net 21 lines and miss coverage. How does that match?

@perlpunk
Copy link
Contributor

https://progress.opensuse.org/issues/189888 says "to simplify our implementation" but you add a net 21 lines and miss coverage. How does that match?

I think the simplification would be to remove the redirect, which is not part of the ticket for some reason

@okurz
Copy link
Member

okurz commented Oct 17, 2025

https://progress.opensuse.org/issues/189888 says "to simplify our implementation" but you add a net 21 lines and miss coverage. How does that match?

I think the simplification would be to remove the redirect, which is not part of the ticket for some reason

Then it still has to be part of the PR then :)

Replace the `url_for` with helper functions which return the domain
name whenever this is applicable based on the setting.

- Link to domain for download assets like iso and `Uploaded
logs` (includes also vars.json)
- Link to domain for the video replay
- No change in the redirection, assuming that this is out of scope
here, and that in general it stays despite the direct link in the
href.

issue: https://progress.opensuse.org/issues/189888
Signed-off-by: Ioannis Bonatakis <[email protected]>
@d3flex d3flex force-pushed the feat/asset_link_to_subdomain branch from 3351dd2 to 86a6c47 Compare October 17, 2025 07:19
@Martchus
Copy link
Contributor

Martchus commented Oct 17, 2025

And probably we even still need the redirect to support old links which might still be present/used on places we don't control.

So I don't think this can simplify this aspect for now but the PR as-is does improve efficiency.

@d3flex
Copy link
Contributor Author

d3flex commented Oct 17, 2025

https://progress.opensuse.org/issues/189888 says "to simplify our implementation" but you add a net 21 lines and miss coverage. How does that match?

I think the simplification would be to remove the redirect, which is not part of the ticket for some reason

Then it still has to be part of the PR then :)

If it is decided to undo the redirect feature, I would rather do it in another PR

@perlpunk
Copy link
Contributor

And probably we even still need the redirect to support old links which might still be present/used on places we don't control.

So I don't think this can simplify this aspect for now but the PR as-is does improve efficiency.

why do we need to redirect these links?
we just deliver the requested file under the regular domain, with the only difference that we make sure it is downloaded and not rendered. which is code which we already have in place in case there is no file domain configured

@Martchus
Copy link
Contributor

we just deliver the requested file under the regular domain, with the only difference that we make sure it is downloaded and not rendered.

If we make sure it is downloaded then we can remove the redirection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants