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

ntopng: fix missing static assets #238162

Merged
merged 1 commit into from
Jun 17, 2023
Merged

Conversation

samhug
Copy link
Contributor

@samhug samhug commented Jun 16, 2023

Instead of building the dist make target in the derivation (requires npm and all that), we're using the upstream pre-built static assets. Upstream refers to these using a sub-module so we need to fetch that.

Without this patch the web UI loads the html, but 404s on all CSS and JS assets because the $out/share/ntopng/httpdocs/dist directory is empty

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

Looks like the patch is missing some bits? And please capitalize first word in commit message body: "instead" -> "Instead".

@bjornfor
Copy link
Contributor

Your PR message explains the situation better than the commit message. Copy it?

Instead of building the `dist` make target in the derivation (requires npm and
all that), we're using the upstream pre-built static assets. Upstream refers to
these using a sub-module so we need to fetch that.

Without this patch the web UI loads the html, but 404s on all CSS and JS assets
because the `$out/share/ntopng/httpdocs/dist` directory is empty
@samhug
Copy link
Contributor Author

samhug commented Jun 17, 2023

Your PR message explains the situation better than the commit message. Copy it?

I've updated the commit message to match the PR

Looks like the patch is missing some bits?

Unsure what the missing bits are unless this was referring to the commit message also. Fetching sub-modules and then updating the source hash to match was really the only change needed.

@ofborg ofborg bot requested a review from bjornfor June 17, 2023 16:09
@bjornfor
Copy link
Contributor

I've updated the commit message to match the PR

Thanks!

Looks like the patch is missing some bits?

Unsure what the missing bits are unless this was referring to the commit message also. Fetching sub-modules and then updating the source hash to match was really the only change needed.

Ok, thanks for clearing that up. (Note to self: it's a bad idea to look at PRs on phone.)

@samhug
Copy link
Contributor Author

samhug commented Jun 17, 2023

Thanks for reviewing the PR!

@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.05:

@github-actions
Copy link
Contributor

Git push to origin failed for release-23.05 with exitcode 1

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

Successfully merging this pull request may close these issues.

2 participants