Modernize scripts in bin/

[hmpf]

Closes #2676

[github-actions]

Test results

     12 files       12 suites   11m 9s ⏱️
3 320 tests 3 320 ✔️ 0 💤 0 ❌
9 435 runs  9 435 ✔️ 0 💤 0 ❌

Results for commit 7dbda65.

♻️ This comment has been updated with latest results.

[hmpf]

This had to be too easy. What did I miss?

[lunkwill42]

As you write, this seemed too easy. There's still some work left, but it's beautiful so far :-)

Yes, moving the scripts is easy, but finding all the references to them, and tests for them is more difficult. In particular, you have some failing tests that look for the binaries under their old names (not the generated tests).

We don't have proper tests that test for consistency when it comes to cron jobs, for example. See all crontab snippets in this directory:

https://github.com/Uninett/nav/tree/d0f3a5bf9c34dec29c6d67da6c8004e28b332f1d/python/nav/etc/cron.d

Also see daemon config in this file:

https://github.com/Uninett/nav/blob/d0f3a5bf9c34dec29c6d67da6c8004e28b332f1d/python/nav/etc/daemons.yml

An extensive search in the codebase for any of the script names that originally ended in .py would be wise.

[hmpf]

Tests for bin/pping.py and bin/smsd updated. main() made importable in bin/ipdevpolld.

Cron-files and daemon.yml updated.

After running ag '\W\w+\.py\W' --ignore "*.js" in order to find all references to py-files, i only found such references to bin-scripts in docstrings and comments. Changed those as well, in their own commit.

[lunkwill42]

Can't put my finger on anything but the failing tests for now. Sudo issues, I assume.

Get the tests running, and I'll approve :)

[hmpf]

The tests are all green when running locally with the test docker setup. I can't seem to make them green on gihub.

What is currently happening is that each script is executed in situ, with hardcoded paths to ./python/nav/bin. It might be that a better solution is testing them in their installed position or via the fact that they are entrypoints in pyproject.toml. Maybe via something like https://github.com/kvas-it/pytest-console-scripts.

[hmpf]

Other ideas:

• https://hg.python.org/cpython/file/default/Lib/test/test_cmd_line_script.py

[lunkwill42]

I was able to reproduce the GitHub issue locally by adding sudo to the tests/docker/Dockerfile image. But then, by looking at what actually changed in this PR, there is no big magic needed to get the tests to work. The installed pping executable is no longer named pping.py, it's named just pping. The only needed change is to remove the .py.

The failing tests are more or less designed to run the installed version of the executable, which is the only "sane" way to ensure sudo-to-root knows how to run it using the correct python interpreter and environment. As a nice "bonus", you also get to verify that the installation of the executable pping shim as defined in pyproject.toml works.

[codecov]

Codecov Report

Attention: Patch coverage is 81.63265% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 56.69%. Comparing base (3c4f8bd) to head (49f747f).

❗ Current head 49f747f differs from pull request most recent head 300d891. Consider uploading reports for the commit 300d891 to get more accurate results

Files	Patch %	Lines
python/nav/bin/radiusparser.py	0.00%	7 Missing ⚠️
python/nav/bin/collect_active_ip.py	90.00%	1 Missing ⚠️
python/nav/bin/servicemon.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
- Coverage   57.18%   56.69%   -0.49%     
==========================================
  Files         568      602      +34     
  Lines       41301    43971    +2670     
==========================================
+ Hits        23617    24931    +1314     
- Misses      17684    19040    +1356     

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

[lunkwill42]

Looks good to me! Glad to get it done!

My last addition is an entry to the release notes, as this change may require action from users on an upgrade.