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

healthchecks: 2.10 -> 3.3 #282912

Merged
merged 6 commits into from
Apr 7, 2024
Merged

Conversation

phaer
Copy link
Member

@phaer phaer commented Jan 22, 2024

Description of changes

Fixed version of automated PR at #282497.
Remove a now unused dependency, add a few missing ones, packaging one of those.
Tests are now passing.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@n8henrie
Copy link
Contributor

Thanks for packaging this. I've been working on it here and there for the past week and have been struggling.

The release notes mention upgrading to django 5, but I wasn't able to figure this out -- pytest-django seems to rely on django 4 and fail. Do you think it would be good to follow upstream in this regard?

@phaer
Copy link
Member Author

phaer commented Jan 23, 2024

@n8henrie: Thanks for your kind words and this helpful hint!

The release notes mention upgrading to django 5, but I wasn't able to figure this out -- pytest-django seems to rely on django 4 and fail. Do you think it would be good to follow upstream in this regard?

Yes, right! The (very basic) nixos integration tests seemed to pass anyway, but I had simply had missed that django_5 exists in nixpkgs (django is still 4.2.9). So just forced pushed and will ask ofborg to run the test.

@phaer
Copy link
Member Author

phaer commented Jan 23, 2024

@ofborg test healthchecks

@n8henrie
Copy link
Contributor

I already tried the same locally but am getting build errors on my aarch64-darwin machine (your prior PR was building at least):

 > ERROR tests/test_fixtures.py::TestLiveServer::test_db_changes_visibility - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_change_settings - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_transactions - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_item - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_url - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_serve_static_dj17_without_staticfiles_app - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > FAILED tests/test_fixtures.py::TestLiveServer::test_specified_port_django_111 - PermissionError: [Errno 1] Operation not permitted
       > FAILED tests/test_fixtures.py::TestLiveServer::test_serve_static_with_staticfiles_app - Failed: nomatch: '*test_a*PASSED*'

@phaer
Copy link
Member Author

phaer commented Jan 23, 2024

@n8henrie Tests are still running in ofborg, but could you elaborate whats your use case of running healthchecks.io on darwin would be? Otherwise I'd propose to just restrict it to Linux, as I am not sure it's intended to run on darwin as all? And as there's no macOS server anymore, it might not be too relevant.

The failing test on aarch64-linux is a problem though. /nix/store/iw4fqx70235rv6dazkfcknvhn1qsflmj-python3.11-pytest-django-4.7.0.drv seems to fail in an shutil call https://logs.ofborg.org/?key=nixos/nixpkgs.282912&attempt_id=b5a3302d-3845-4304-b1b0-e1d820f92ee4

@n8henrie
Copy link
Contributor

I guess that's an interesting question. I'm about 50/50 linux / darwin day-to-day. Healthchecks is python / django and specifically includes MacOS-specific installation instructions -- turning the question around, why wouldn't one expect this package to build on macOS?

And as there's no macOS server anymore, it might not be too relevant.

Plenty of people running Mac Minis as dandy little servers (and I'd argue that a used / refurb M1 mini can be pretty darn competitive in terms of cost / performance / power usage). nix-darwin and home-manager both work really well for configuring launchd agents.

@phaer
Copy link
Member Author

phaer commented Jan 23, 2024

turning the question around, why wouldn't one expect this package to build on macOS?

Because maintainer time is finite :) I am happy if it works on macOS, I just can't commit to investing much time into that myself. So in the context of this PR: Did 2.1 run on darwin?

@n8henrie
Copy link
Contributor

Did 2.1 run on darwin?

Yes, works fine:

$ sw_vers
ProductName:            macOS
ProductVersion:         14.2.1
BuildVersion:           23C71
$ DB_NAME=./hc.sqlite STATIC_ROOT=./result/opt/healthchecks/static ./result/opt/healthchecks/manage.py runserver
Watching for file changes with StatReloader
Performing system checks...

System check identified no issues (0 silenced).
January 23, 2024 - 23:30:17
Django version 4.2.7, using settings 'hc.settings'
Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.

@n8henrie
Copy link
Contributor

I'll take a look at the test failure

@n8henrie
Copy link
Contributor

failing test on aarch64-linux is a problem though

I'm running it (including the django_5 commit) on a on a Pi4 and it seems to work great.

@phaer
Copy link
Member Author

phaer commented Jan 24, 2024

@n8henrie The ofborg test on aarch64-linux failed in https://github.com/NixOS/nixpkgs/pull/282912/checks?check_run_id=20794046357 . But nice it works for you, so just the darwin thing to figure out

@n8henrie
Copy link
Contributor

n8henrie commented Jan 26, 2024

Hmmm. All nix's levels of indirection make debugging this harder.

pytest-django is getting a permission error trying to bind a socket.

I'm not sure what behavior would have changed here between django 4 and 5.

@phaer
Copy link
Member Author

phaer commented Jan 26, 2024

@n8henrie I can reproduce test failures in pytest-django on aarch64-darwin by running just
nix build .#python3.pkgs.pytest-django. So it seems pytest-django is broken on aarch64-darwin on master. The failing ofborg test on aarch64-linux seems to suggest it's broken there as well which makes my curios why that is.
Anyway, that looks a separate issue to file & debug.

@n8henrie
Copy link
Contributor

Thanks, that helps a lot to know -- my local checkout was building .#python3.pkgs.pytest-django fine... so I don't see why my checkout would be fine for that but fail on pytest-django when building healthchecks.

Let me pull and rebase and take another look.

@n8henrie
Copy link
Contributor

Still building fine. Just now:

$ nix build github:nixos/nixpkgs/master#python3.pkgs.pytest-django
$ echo $?
0

I must be doing something wrong here. Will try with substituters disabled.

@n8henrie
Copy link
Contributor

Hmmm, sandbox issue.

I can now reproduce the pytest-django failure (with --rebuild)

$ nix build --rebuild .#python311.pkgs.pytest-django
error: builder for '/nix/store/l35qps1mk6nxhwwa9ijblsk6xw8q2fnf-python3.11-pytest-django-4.7.0.drv' failed with exit code 1;
       last 10 log lines:
       > ERROR tests/test_fixtures.py::TestLiveServer::test_change_settings - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_transactions - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_serve_static_dj17_without_staticfiles_app - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_db_changes_visibility - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_item - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > ERROR tests/test_fixtures.py::TestLiveServer::test_url - socket.gaierror: [Errno 8] nodename nor servname provided, or not known
       > FAILED tests/test_fixtures.py::TestLiveServer::test_specified_port_django_111 - PermissionError: [Errno 1] Operation not permitted
       > FAILED tests/test_fixtures.py::TestLiveServer::test_serve_static_with_staticfiles_app - Failed: nomatch: '*test_a*PASSED*'
       > ============= 2 failed, 202 passed, 2 skipped, 10 errors in 25.24s =============
       > /nix/store/ymqnr6q4slam0ns76mjpcjry70fa8h3i-stdenv-darwin/setup: line 1591: pop_var_context: head of shell_variables not a function context
       For full logs, run 'nix log /nix/store/l35qps1mk6nxhwwa9ijblsk6xw8q2fnf-python3.11-pytest-django-4.7.0.drv'.
$
$ nix build --rebuild .#python311.pkgs.pytest-django --option sandbox false
$ echo $?
0

@n8henrie
Copy link
Contributor

And agree that the problem looks isolated to that package; after a successful (due to no-sandbox) build of pytest-django, this PR works a treat:

$ nix build --rebuild  .#healthchecks
$ echo $?
0

So this seems good to go to me. I'll look for / open a separate issue. Thank you again!

@n8henrie
Copy link
Contributor

n8henrie commented Jan 28, 2024

Now that #284401 is merged, after a rebase:

$ nix build --rebuild .#healthchecks
$ echo $?
0

FWIW I've been running hc-3.1 from this PR on my Pi for the past few days and it's been working brilliantly -- thank you again!

@n8henrie
Copy link
Contributor

n8henrie commented Feb 1, 2024

Setting settings.DEBUG = true; leads to an error in the systemd service:

Jan 31 16:05:49 homeslice healthchecks-pre-start[417515]: CommandError: Compressor is disabled. Set the COMPRESS_ENABLED setting or use --force to override.
Jan 31 16:05:49 homeslice systemd[1]: healthchecks.service: Control process exited, code=exited, status=1/FAILURE

Adding settings.COMPRESS_ENABLED = "True" (or "False") doesn't seem to do anything to rectify the error.

One can still run the debug server manually with: e.g. sudo -u healthchecks healthchecks-manage runserver 0.0.0.0:8888

@phaer
Copy link
Member Author

phaer commented Feb 1, 2024

@n8henrie This might be due to us calling manage.py compress during service startup, which might might not be supported in debug mode.

can you check whether it works as expected for you if you replace that line with
${lib.optionalString !cfg.settings.DEBUG "${pkg}/opt/healthchecks/manage.py compress"}?

@n8henrie
Copy link
Contributor

n8henrie commented Feb 2, 2024

Yes, I assumed the same but didn't have time to test that day.

Unfortunately ran into a bunch of infinite recursion issues trying to use fetchers from pkgs for the PR and then import the module and then a bunch of network errors trying to use builtins.fetchGit but eventually got it to work.

Can confirm that with this diff debug mode is working:

Note that DEBUG is not a boolean due to apply = boolToPython;, and I removed buildEnv as it was unused.

diff --git a/nixos/modules/services/web-apps/healthchecks.nix b/nixos/modules/services/web-apps/healthchecks.nix
index 1d439f162313..d0a211992fc1 100644
--- a/nixos/modules/services/web-apps/healthchecks.nix
+++ b/nixos/modules/services/web-apps/healthchecks.nix
@@ -1,4 +1,4 @@
-{ config, lib, options, pkgs, buildEnv, ... }:
+{ config, lib, options, pkgs, ... }:
 
 with lib;
 
@@ -213,8 +213,7 @@ in
           preStart = ''
             ${pkg}/opt/healthchecks/manage.py collectstatic --no-input
             ${pkg}/opt/healthchecks/manage.py remove_stale_contenttypes --no-input
-            ${pkg}/opt/healthchecks/manage.py compress
-          '';
+          '' + lib.optionalString (cfg.settings.DEBUG != "True") "${pkg}/opt/healthchecks/manage.py compress";
 
           serviceConfig = commonConfig // {
             Restart = "always";

@n8henrie
Copy link
Contributor

@phaer any other thoughts / need any help?

Looks like 3.2 is also out: #288494

@github-actions github-actions bot removed 6.topic: vim 6.topic: erlang 6.topic: ocaml 6.topic: fetch 6.topic: stdenv Standard environment 6.topic: nodejs 6.topic: pantheon The Pantheon desktop environment 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: lua 6.topic: testing Tooling for automated testing of packages and modules 6.topic: module system About "NixOS" module system internals 6.topic: systemd 6.topic: agda 6.topic: vscode 6.topic: lib The Nixpkgs function library 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php labels Apr 5, 2024
@phaer
Copy link
Member Author

phaer commented Apr 5, 2024

Not sure what happened here: I had rebased (on master) and force-pushed after updating to 3.3.
Updating the web UI took a long time, but github had troubles at the time. Thankfully it seemingly did not trigger notifications to hundreds of people this time. Rebasing again on a slightly newer master and pushing removed other ppls commits from this PR again.

@bhankas
Copy link
Contributor

bhankas commented Apr 5, 2024

Latest changes look good, nixpkgs-review also satisfied:

5 packages built:
healthchecks python311Packages.oncalendar python311Packages.oncalendar.dist python312Packages.oncalendar python312Packages.oncalendar.dist

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1574

@SuperSandro2000 SuperSandro2000 merged commit 0f3f375 into NixOS:master Apr 7, 2024
25 checks passed
@phaer phaer deleted the healthchecks-update branch April 7, 2024 20:14
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.

8 participants