Skip to content

Update the README to match the code - #56

Open
somethingwithproof wants to merge 1 commit into
developfrom
docs/readme-verified
Open

somethingwithproof wants to merge 1 commit into
developfrom
docs/readme-verified

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Description

The README describes the 2007 ExtJS plugin, not the code on develop. This change makes each statement match the default branch as of origin/develop (66b2cf3), without touching code. Statements that are wrong are corrected or removed, missing requirements and behaviour are added, and statements I could not check are left as they were and listed below.

The largest corrections:

  • The user interface is jQuery (js/npc.js), not ExtJS, and it has seven screens. The context menus, drag and drop columns, portlets, graph icons, toggling of check marks, per user layouts and the N2C import screen that the README described do not exist.
  • Opening the NPC tab currently ends in a fatal error. npc.php does not load controllers/controller.php, so every page request fails with Class "Controller" not found, and Cacti then disables the plugin. The README now says so in a Status section. The hardening PR (Harden the NPC controllers and fix review findings #54) fixes it; when that merges, the Status section should be removed.
  • Remote commands are no longer open to every NPC user: they need the NPC Global Commands realm, which the README never mentioned.
  • Cacti 1.2.1 or later, PHP 8.2 or later, and the realms, settings (with defaults), upgrade and uninstall behaviour, command line options, log location, translation workflow and test commands are documented.

Related Issue

No open issue.

Scope and risk

  • Scope: README.md and a CHANGELOG line. INFO and manifest.json were checked and are correct (below), so they are unchanged. No code is touched.
  • Risk: documentation only. The wording of the Status section and the "does not work" statements about the command dialog and perfdata.php describes defects that exist today. If the defects are fixed first, those sentences have to be removed with the fix; the hardening PR fixes most of them and the README then needs a follow-up edit.
  • Compatibility choice: where code and old README disagree I documented the code. I did not change behaviour to match the old text.

Rollback

Revert the commit.

How the claims were verified

All runs used the code of origin/develop with this README. Environment: PHP 8.4, MariaDB 11.8.9 in a throwaway container, Cacti 1.2.32 (the 1.2.x branch) with its own dependencies for the web checks (a second Cacti 1.3.0 tree for the test run), PHP's built-in web server, and a logged-in test user holding the realms needed for each page. "Read" means I read the code; "ran" means I executed it.

Claim in the README Result Evidence
Cacti 1.2.1 or later (was "Cacti 1.2.1") Confirmed, scoped to the 1.2 series INFO:compat = 1.2.1; .github/workflows/plugin-ci-workflow.yml:38 CACTI: 1.2.x. Nothing in the repository shows 1.2.1 itself being tested; only 1.2.x is.
PHP 8.2 or later; CI runs 8.2, 8.3, 8.4 (new) Added tests/Security/PhpCompatibilityTest.php:27-28 (floor 8.2), workflow matrix php: ['8.2', '8.3', '8.4'] (line 52). I could not run PHP 8.2 here; the floor is the one the repository states.
No PHP extension beyond Cacti's (new) Added Read: no mb_, curl_, iconv, xml, gd, ldap, snmp, gmp, openssl, intl calls outside tests/; the only process call is exec() in controllers/sync.php:77.
php in the web user's PATH for the host import (new) Added controllers/sync.php:77 exec('php ' . $path ...).
NDOUtils 1.4b7, Nagios 3.x Not verifiable, unchanged Needs a Nagios and NDOUtils install. The table definitions in includes/database.php follow the NDOUtils layout with the npc_ prefix.
PHP memory_limit may be too low Not verifiable, unchanged No measurement made.
git clone https://github.com/Cacti/plugin_npc.git npc into plugins Confirmed git remote -v shows Cacti/plugin_npc; the directory name must be npc because setup.php:58 reads plugins/npc/INFO (clarified in the text).
Install and enable under Console > Configuration > Plugins Confirmed, wording fixed Ran in Cacti 1.2.32: menu entry Configuration > Plugins (lib/plugins.php:1564 of Cacti); the row is named "Npc", described as "Nagios Plugin for Cacti", with the icons "Install Plugin" or "Enable Plugin" and "Uninstall Plugin" and the status "Installed". The README said "link".
plugin_manage.php --install --enable (new) Added Ran: php cli/plugin_manage.php --plugin=npc --install printed "installed successfully", plugin_config.status 4 (Installed); --enable set it to 1 and all eight hooks to status 1.
Install creates 62 tables, one host column, two realms, three settings (new) Added Ran: 62 npc_% tables in information_schema, host.npc_host_object_id int(11), plugin_realms rows npc.php "NPC" and npc1.php "NPC Global Commands", settings rows npc_date_format=Y-m-d, npc_time_format=H:i, npc_log_level=0. includes/database.php:44, :69-71, setup.php:66-67.
Disable and uninstall (new) Added Ran: --disable then --uninstall left 0 npc_% tables, no npc_host_object_id column, 0 realms, 0 npc_% settings, no plugin_config row (setup.php:102-170). Reinstall with --install --enable worked.
Realms: NPC and NPC Global Commands, granted on the user's Permissions tab (new; the old text said every NPC user can execute commands) Changed Read: controllers/nagios.php:172 checks npc1.php before any command. Read: user_admin.php of Cacti 1.2.32 has the tab "Permissions" listing the realms under Console > Configuration > Users.
NPC options under Console > Configuration > Settings Confirmed Ran: settings.php?tab=npc shows the tab "NPC" with ten fields.
Remote Commands (checkbox) Confirmed, default added Ran: unchecked after install. controllers/nagios.php:156.
Nagios Command File Path Confirmed, requirement added Ran: text field, 255 characters. nagioscmd.php:1681-1688 requires the file to exist and be readable and writable.
Nagios URL "used to get the status map and reporting, 'Nagios' link in the navigation pane" Changed Read: the value is only written into npcConfig (controllers/layout.php:49); js/npc.js reads only csrfToken, npcUrl, userName, portletRefresh and currentTab (grep -o 'cfg\.\w*' js/npc.js). There is no Nagios link or iframe.
Date and time format Changed Ran: select options are nine date and six time formats; default date Y-m-d; the stored default time H:i is not one of the options. Read: the settings are only used by controllers/nagios.php:316 (Process Info) and by detail formatters that no screen calls.
Portlet Refresh Rate Added Ran: text field, three characters, default 60; help text says minimum 30. Read: js/npc.js:62 interval || cfg.portletRefresh || 60, no minimum.
Host/Service Config Type Added Ran: select 0 or 1, default 1. Read: controllers/controller.php:252 and the config_type = ? conditions in the host and service queries.
Host Icons, Service Icons Added Ran: two checkboxes. Read: the values reach npcConfig and no code uses them.
Logging Level Added Ran: select None (default), Error, Warn, Info, Debug. Read: controllers/controller.php:400-416 logs to the Cacti log with component NPC.
Features 1 to 6 of the old list (ExtJS, complete replacement, N2C, per user customisation) Changed js/npc.js:4 "jQuery UI frontend replacing ExtJS 2.2"; tests/Security/NoExtJSDependencyTest.php. controllers/layout.php:18-26 lists seven tabs. No code in js/ calls the sync or settings modules.
JSON loaded asynchronously, no reloading Changed Read: js/npc.js:25-30 $.getJSON; tab links are normal links (controllers/layout.php:34), so a tab change loads a page. Auto refresh only for Hosts (js/npc.js:237), Services (:298) and Event Log (:390).
Usage tips: double click for details, right click menus, graph icon, toggling check marks, column drag and hide, portlets, per user layout, N2C Removed and replaced Read: no such handlers in js/npc.js; details open on a single click of the name (:226, :287), commands from a gear icon (:220, :231), a comment is deleted by the cross (:315, :332).
Lists show the first 25 rows (new) Added Read: controllers/controller.php var $limit = 25 and js/npc.js sends no start or limit.
Command dialog options (new) Added Read: the dialog sends only host, service, author and comment (js/npc.js:498-590); nagioscmd.php requires sticky, notify, persistent, times and so on for Acknowledge, Add Comment, Schedule Downtime and Schedule Check. Ran a script that extracts the required parameters from nagioscmd.php. I could not send a command through the page because of the fatal error below.
The NPC tab fails with a fatal error and the plugin is disabled (new Status note) Added Ran: npc.php in Cacti 1.2.32 returned Fatal error: Uncaught Error: Class "Controller" not found in controllers/layout.php on line 3; plugin_config.status went from 1 to 4 and cacti.log shows "ERRORS DETECTED - DISABLING PLUGIN 'npc'". Nothing requires controllers/controller.php except config.php (grep -rn controller.php).
Settings tab, device mapping and scripts are unaffected (new) Confirmed Ran: settings.php?tab=npc renders; host.php?action=edit shows "Nagios Host Mapping"; the scripts below ran.
NPC tab in the console header (new) Added Ran: index.php contains the link to plugins/npc/npc.php. The graph view header did not show one.
Upgrade behaviour (new) Added Ran: set plugin_config.version to 3.0 and opened settings.php?tab=npc. The version became 3.1, npc_settings was reset and re-seeded, and the plugin directory lost tests/ and phpunit.xml. setup.php:349-369, npc_prune_files() at setup.php:531. The same run logged an SQL error for the table npc_contracts (a typo for npc_contacts in npc_upgrade_tables()), which the hardening PR fixes.
Without NDO2DB the screens are empty Confirmed Ran with empty tables: the scripts print only headers.
perfdata.php can be polled by Cacti (old text) Confirmed, details added Ran with fixture rows: php perfdata.php --type=service --id=777 printed load1:0.52 load5:0.40 : (newest check); --type=service --host=web1 --service=Load the same; --type=host --id=778 printed the oldest of two stored checks.
Script options (new) Added Read the option parsers: cli.php:34-86, perfdata.php:27-60. Ran each without arguments and with --list-hosts, --list-hostgroups, --list-services, --perfdata, --perf-history. The usage text of both names --datasource, which neither parses; --ds is read into $ds and never used (grep -n '\$ds\b').
ndo2db and ndomod settings, inittab notes Not verifiable, unchanged Needs NDOUtils. The db_prefix=npc_ value matches the table names.
Skip the "initializing the SQL database" section Confirmed Ran: the plugin installer creates all tables.
Translations (new) Added .github/copilot-instructions.md says Weblate owns the .po and .mo files and a PR stages only cacti.pot. Ran locales/build_gettext.sh in a copy: it works through xgettext, msgmerge and msgfmt, reads include/cacti_version (absent in the plugin directory, so the package version stays empty, as in the committed cacti.pot), and the sed -i calls fail with the BSD sed of macOS (observed), so GNU sed is listed.
Development and test commands (new) Added Ran exactly as written, in the CI order. include/vendor/bin/pest --configuration=plugins/npc/phpunit.xml --coverage-clover=plugins/npc/coverage/clover.xml: 98 passed, 1 skipped. php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100: "no measured lines changed", exit 0. php tests/bin/check-i18n-pot.php origin/develop: no changes detected, exit 0. php tests/bin/validate-manifest.php: matches, exit 0. The .cacti-version rule is tests/bootstrap-unit.php:65.
INFO (name, version = 3.1, compat = 1.2.1) and manifest.json Confirmed, unchanged validate-manifest.php passes; CHANGELOG.md has --- 3.1 --- below --- develop ---; Cacti listed version 3.1 and name "Nagios Plugin for Cacti". The capabilities line was not checked.
Bug reporting paragraph Fixed "If you find a first search" was missing the word "bug".

Not checked: the NDOUtils and Nagios versions, the memory_limit advice, the Nagios configuration values, and the capabilities entry in INFO. They are unchanged.

Test run against both core refs

The README is the only changed file, so the code results are the same as origin/develop. In CI order (plugin install with php cli/plugin_manage.php --plugin=npc --install --enable, php -l on every PHP file, Pest with coverage, patch coverage, translation template check, manifest check, cacti.log grep):

Core ref Install php -l Pest Patch coverage i18n and manifest cacti.log grep
1.2.32 (1.2.x) clean clean 98 passed, 1 skipped no measured lines changed pass no matching line
develop (1.3.0) clean clean 98 passed, 1 skipped no measured lines changed pass one Cacti-core warning from the --uninstall step of my simulation, which the workflow does not run

I did not run PHP 8.2 or 8.3 or a fresh composer install of the 1.2.x dependencies; the Pest version comes from the develop lock file.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Documentation only.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation. (this is the documentation change)
  • I have updated the documentation repository accordingly.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant