fix!: rebuild the unit test suite, document lib/, fix the bugs it found

[tas50]

Rebuilds the test suite around one spec file per lib file, documents every method in lib/ with YARD, and fixes the bugs the new coverage exposed.

Rebased on main (through #248) and updated for review. See Review round 1 at the bottom for what changed.

Release

This ships as 8.0.0. Three behaviours change in ways that can fail a converge which previously succeeded, so the PR title carries fix!: and the commit carries a BREAKING CHANGE: footer — Release Please will pick up the major bump from either.

Change	Before	After
user_data pointing at a nonexistent file	read as nil; server booted with no cloud-init, silently	ActionFailed
security_groups set to a bare String	dropped; server booted into the default group	ActionFailed
ssl_ca_file (OS_CACERT / clouds.yaml cacert)	parsed, then discarded — the README's documented mapping never took effect	actually passed to Excon

That last one deserves a release note of its own: a stale or wrong CA path that was previously ignored is now enforced, so a deployment carrying one will start failing TLS on upgrade until it is corrected or removed.

Two smaller changes, neither of which should break a working converge:

• image_ref/flavor_ref: a purely numeric ref now matches an id before a name. On clouds where Nova returns Integer ids the id branch could never match a String ref, so a resource named "2" won. A flavor named "2" alongside a different flavor with id 2 now resolves to the latter — which is what the documented id-first contract says should happen.
• A block_device_mapping timeout that is not a number now raises ActionFailed naming the key, and is checked before the Cinder volume is created rather than after.

Tests

	before	after
examples	141	360
runtime	20.19s	0.31s
line coverage	not measured	100%
spec files	3	8

• One spec file per lib file. The 1442-line openstack_spec.rb monolith is split so the spec layout mirrors lib/.
• verify_partial_doubles is on, along with random ordering, aggregate_failures, and disable_monkey_patching!. Fog model objects use instance_double; Fog service objects can't, because Fog defines servers/list_networks/etc. dynamically at instantiation — spec/support/fog_doubles.rb documents why rather than silently downgrading.
• Real files instead of File.exist? stubs in the clouds.yaml specs, via Dir.mktmpdir. The search-path logic is now actually exercised.
• The environment cannot leak in. ENV is replaced wholesale with an OS_*-free hash, and the clouds.yaml specs additionally pin Dir.pwd, Dir.home and /etc/openstack. Verified by running the suite against a planted ~/.config/openstack/secure.yaml and exported OS_CLOUD/OS_USERNAME/OS_CACERT: 360 pass, and no planted secret reaches the output.
• wait_for doubles follow Fog's contract — the readiness block is evaluated in the model's own context, and a block that never goes truthy raises Fog::Errors::TimeoutError.
• Nothing sleeps. The old 20s runtime was countdown and wait_for doing real wall-clock waits.
• SimpleCov reports to coverage/ with no enforced threshold. On in CI, opt-in locally via rake coverage or COVERAGE=1, so a single-file run doesn't pay for instrumentation. The five uncovered branches are all require x unless defined?(X) guards.

Verified order-independent across 5 seeds.

The suite was checked by mutation testing

Coverage says which lines ran, not whether anything would notice them breaking. So 40 deliberate mutations were applied to lib/ and the suite re-run against each: 38 were killed, and the 2 survivors are provably equivalent mutants (return if merged.empty?, whose removal cannot change behaviour because an empty merged implies an empty cc; and one guard whose surviving form was then made distinguishable and is now killed too).

That exercise earned its keep — it caught a genuinely untested guard. single.name && regex.match?(single.name) looked interchangeable with regex.match?(single.name.to_s), and is, for every regex that doesn't match the empty string. For /.*/ it isn't: the to_s form silently selects an unnamed Neutron network. There is now an example pinning that.

Bugs fixed

Each has a regression test, marked with a # Regression: comment explaining the failure mode. Every one of those comments was re-checked against the pre-PR code in this round.

volume.rb

• volumes.first { |x| x.id == vol_id } — Array#first silently ignores blocks. Creation waited on whichever volume was listed first, not the one just created. Now fetched by id with volumes.get, a direct GET, so it is correct at any volume count — a listing returns only one page.
• get_bdm mutated config[:block_device_mapping] in place, so a retried create saw an already-stripped hash.
• creation_timeout / attach_timeout are Strings when quoted in YAML; "5" > 0 raised ArgumentError. Parsed in base 10 so "010" is ten seconds and not eight, validated before the Cinder volume exists, and reported as ActionFailed naming the key.
• Errors are Kitchen::ActionFailed, which create's rescue Fog::Errors::Error, Excon::Errors::Error actually catches.

config.rb

• server_name_prefix called gsub! on its argument — rewrote config[:server_name_prefix] in place, and raised FrozenError on a frozen literal.
• Etc.getpwuid raises for a uid with no passwd entry rather than returning nil, so the "nologin" fallback was dead code and the container case crashed.
• Etc and Socket were used without being required.
• The per-component name budgets are now derived from MAX_SERVER_NAME_LENGTH, so the 63-character limit is maintained by the code rather than described by a constant nothing read.

networking.rb / openstack.rb

• Three places indexed straight into [0]["id"] on a Neutron response — the floating IP pool lookup, the named-network lookup, and the floating IP release in destroy. All three now raise ActionFailed with the offending name, and the release path warns and continues.
• parse_ips used select!, and Array(x) returns x itself when x is already an Array — so it filtered the Fog server model's own address lists in place.
• free_ip_from_pool built and compacted a whole throwaway array while holding IP_POOL_LOCK just to take the first element.
• ssl_ca_file is an Excon connection option, not a Fog one. See the release table above.

server_helper.rb

• A missing user_data file silently became nil. Now raises. (Breaking — see above.)
• A security_groups value that wasn't an Array was silently dropped. Now raises. (Breaking — see above.)
• Config is validated before get_bdm runs. get_bdm is the only step in create_server that creates a resource, and a volume it created was orphaned when a later purely-local check raised — the new volume's id lived only in the server definition being discarded, so kitchen destroy could never reach it.
• Resource ids are compared as strings (Nova returns integer flavor ids on some deployments).

clouds.rb

• The SSL ternary read env[:ssl_verify_peer], which ENV_VAR_MAP can never produce. Dead branch removed.
• A malformed or non-mapping clouds.yaml reports the file by name, and a named cloud entry that is present but is not a mapping is now reported too, rather than proceeding with nil credentials into an opaque Keystone failure.
• Dir.home raises when HOME is unset and the uid has no passwd entry. With OS_CLOUD set, the search crashed before /etc/openstack — the one location such a container is likely to have — was tried.
• YAML.safe_load_file now that Ruby 3.1 is the floor.

Docs

• YARD tags on all 57 methods and 18 constants — yard stats reports 100% documented.
• .yardopts uses --private: the driver's public surface is only create/destroy, so default YARD would render a two-method site.
• rake yard, rake yard_stats and rake coverage added. None is wired into rake default or CI, per the no-gating requirement.

README

Rewritten for someone arriving new: a quick start that goes end to end, a full configuration reference grouped by task, worked examples, and a troubleshooting table keyed on the actual error strings the driver raises.

Every option was extracted from diagnose.keys + Fog::OpenStack::Compute.recognized + required_server_settings, and the finished tables were diffed back against that set — no invented options, and all 59 real ones either tabled or named in prose.

Three options that accept input and do nothing

All three are documented as ineffective rather than changed, since fixing them is a behavior change:

Option	Status
no_ssh_tcp_check	Declared via default_config, read nowhere in lib/.
no_ssh_tcp_check_sleep	Same.
pre_create_command	Declared by Test Kitchen's Driver::Base, but this driver overrides create without calling it or super.

Left alone in this PR by agreement — @ramereth is following up separately on removing vs. wiring them.

Verification

$ bundle exec rake coverage && bundle exec rake style
360 examples, 0 failures
Line Coverage: 100.0% (474 / 474)
Branch Coverage: 97.19% (173 / 178)
11 files inspected, no offenses detected

$ bundle exec yard stats
100.00% documented

Review round 1

All 27 inline comments addressed. Highlights:

• volume_device_name → device_name in the boot-from-volume example. Confirmed against fog-openstack-1.1.5/compute/requests/create_server.rb: the v1 mapping reads exactly :delete_on_termination, :device_name, :volume_id, :volume_size.
• wait_for_volume uses volumes.get(vol_id), passed the already-built service object so there's no second Keystone round-trip, and raises ActionFailed rather than RuntimeError.
• timeout_value parses base 10, wraps ArgumentError/TypeError into ActionFailed naming the key, and both timeouts are validated before the Cinder create. One correction: the suggested Integer(value, 10) can't be applied literally — a base argument is only legal for a String, so Integer(30, 10) raises base specified for non string value. The existing suite caught it immediately. It's now value.is_a?(String) ? Integer(value, 10) : Integer(value).
• The Regexp#match?(nil) claim was wrong and is gone. Verified: /ubuntu/.match?(nil) # => false. The comment and the PR description bullet both said it raised TypeError; neither does now. The guard stays, on the unnamed-networks rationale — and mutation testing then showed it is load-bearing for a regex matching "", which now has its own example.
• Spec isolation is real, not claimed. OS_CLIENT_SECURE_FILE is pinned, and Dir.pwd/Dir.home//etc/openstack are pinned too — a nonexistent env path alone doesn't block the search, it just falls through. Verified against a planted secure.yaml.
• openstack_version_spec keeps its two repo-root reads, which are now the documented exemption in AGENTS.md and CONTRIBUTING rather than a contradiction of them, and each skips when the file is absent so a packaged gem still passes.
• The vacuous tests are gone. The parse_ips example asserted on a .dup; it now asserts on the object passed in — and parse_ips was indeed mutating, so the code changed too. The STRING_CONFIG_KEYS example recomputed the constant's own defining expression; it now pins a literal list.
• fog_doubles' wait_for runs the caller's block and raises TimeoutError when it never goes truthy, and addresses is consistent with public_ip_addresses. Two knock-on findings: the get_ip timeout test now drives the real predicate instead of stubbing wait_for to raise, and the block's sleep(1) was reaching Kernel#sleep on the server (the driver's sleep stub never applied, because wait_for instance_execs on the model) — the suite briefly went from 0.31s to 12.4s until that was stubbed on the right object.
• attach_ip_from_pool's test description now says what it guarantees: get_ip reuses the value; destroy rebuilds the address from the server and :public_ip_order and never reads config[:floating_ip].
• Also done: find instead of map+compact under IP_POOL_LOCK; extract_cloud raises on a present-but-non-mapping entry; Dir.home guarded; YAML.safe_load_file; MAX_PREFIX_LENGTH derived; @return [void] on disable_ssl_validation; SimpleCov guarded behind CI/COVERAGE; stub_env hoisted into the shared context; the clouds tmpdir made lazy; the OS_INSECURE phrasing corrected.

Not done, by agreement: the ingest-side coercion layer in clouds.rb. The drift is real and reproduces — identity_api_version: 3 becomes "3" in config (what kitchen diagnose prints) but "v3" at the Fog boundary, and openstack_tenant/openstack_tenant_id are in FOG_STRING_SETTINGS but not STRING_CONFIG_KEYS. Consolidating it changes diagnose output, which is a separable behaviour change, and this PR is already large. Happy to take it as a follow-up.

Attribution: deliberate. The OSU copyright on the new spec files matches the six lib/ files that already carry it on main.

🤖 Generated with Claude Code

[ramereth]

Overall: strong PR — the test-suite rewrite is a big upgrade and most of the bug-fix claims verified cleanly when checked against Ruby itself, fog-openstack 1.1.5, Excon, and the openstacksdk docs. CI is green on 3.1–4.0. A handful of things need fixing before merge (inline comments; summary below), plus one release-process question.

Verified correct (checked against upstream sources, not just read)

• ssl_ca_file via connection_options is the right mechanism: fog-openstack passes connection_options to both the Keystone token request and the service connection (core.rb), and ssl_ca_file is in Excon's VALID_CONNECTION_KEYS.
• The removed env[:ssl_verify_peer] branch was genuinely dead — nothing in ENV_VAR_MAP ever produced that key.
• Array#first ignores blocks, Etc.getpwuid raises ArgumentError for a uid with no passwd entry, and "5" > 0 raises — all three fixes are legitimate.
• The clouds.yaml claims (search paths, secure.yaml merge, OS_CLIENT_CONFIG_FILE/OS_CLIENT_SECURE_FILE, verify/cacert) match the openstacksdk configuration docs.
• The README option tables match Fog::OpenStack::Compute.recognized (I ran it), and the dead-option claims (no_ssh_tcp_check, no_ssh_tcp_check_sleep, pre_create_command) check out by grep.

Must fix (details inline)

1. README boot-from-volume example: volume_device_name is not a key fog-openstack reads — regression vs the current README's device_name.
2. wait_for_volume: use volumes.get(vol_id) rather than .find over one list page; the new RuntimeErrors also escape create's Fog/Excon rescue as raw backtraces.
3. timeout_value: bare Integer() octal footgun, plus unwrapped ArgumentError/TypeError raised after the volume already exists (leak).
4. The Regexp#match?(nil) "regression" isn't real — it returns false, it does not raise. The PR description bullet and the spec comment need correcting.
5. Spec isolation: the secure.yaml search path is unpinned, so a developer's real ~/.config/openstack/secure.yaml / /etc/openstack can leak into the suite; and openstack_version_spec violates the "never read outside Dir.mktmpdir" invariant this PR itself adds to AGENTS.md.
6. A few tests are vacuous or pin impossible states (the parse_ips mutation test, fog_server's wait_for stub).

Versioning / release

This PR deliberately breaks three behaviors: a missing user_data file now raises, a non-Array security_groups now raises, and — easy to miss — a stale OS_CACERT/cacert that was previously parsed-then-dropped will now actually be handed to Excon, which can start failing TLS for users who never noticed it was set. I agree with all three, but main is at 7.0.1 and the commit headlines carry no conventional-commit type, so Release Please can't classify them. They should ride a fix!:/feat!: (→ 8.0.0), or we consciously accept them as fixes with prominent release notes — needs a decision before merge, and the PR title/squash commit needs a conventional type either way.

On the three inert options you flagged for a decision (no_ssh_tcp_check, no_ssh_tcp_check_sleep, pre_create_command): leaving them untouched in this PR was the right call — I'll follow up separately on removing vs. wiring them.

🤖 Review compiled with Claude Code

[tas50]

Rebased on main (now through #248 — the merge base was already #246, so only the renovate and changelog commits were new) and all 27 inline comments are addressed. Replies are on each thread; resolved everything except the coercion-layer one, which I'm deliberately leaving open as a follow-up.

Versioning — this is 8.0.0

Decided rather than deferred. The PR title is now fix!: rebuild the unit test suite, document lib/, fix the bugs it found, so the squash commit carries a conventional type and the breaking marker, and the commit itself carries a BREAKING CHANGE: footer. Release Please takes the major bump from either, so it lands as 8.0.0 whichever way it's merged.

The footer names all three breaks, including the ssl_ca_file one — a stale OS_CACERT that was previously parsed-and-dropped is now enforced and can start failing TLS on upgrade. There's a table at the top of the description for the release notes, plus the two smaller changes (numeric-ref id precedence, and the timeout raise) called out as worth mentioning but not converge-breaking.

The five "must fix" items

1. volume_device_name → device_name. Confirmed against create_server.rb — the v1 mapping reads exactly the four keys you listed. The same wrong key was in two volume_spec.rb fixtures; those are fixed too.
2. wait_for_volume uses volumes.get(vol_id), passed the service object built in create_volume so there's no second Keystone round-trip, and raises ActionFailed throughout.
3. timeout_value — see the note below; fixed, with one correction to the suggestion.
4. The Regexp#match?(nil) claim was wrong and is gone from both the spec comment and the description.
5. Spec isolation — OS_CLIENT_SECURE_FILE pinned, and Dir.pwd/Dir.home//etc/openstack pinned with it, because a nonexistent pinned path alone just falls through to the rest of the search. openstack_version_spec keeps its two reads as a documented exemption that also skips when the files are absent.
6. The vacuous tests are gone — and parse_ips really was mutating, so the code changed as well as the test.

Two corrections worth flagging

Integer(value, 10) can't be applied literally. The base argument is only legal for a String, so an unquoted creation_timeout: 30 raises ArgumentError: base specified for non string value. The existing suite caught it on the first run. It's value.is_a?(String) ? Integer(value, 10) : Integer(value) now, rescued into ActionFailed.

The single.name && guard is load-bearing after all. You're right that match?(nil) returns false and that the guard is behaviour-identical for /ubuntu/. It isn't for a regex matching the empty string — regex.match?(nil.to_s) hands "" to /.*/, which matches, silently selecting an unnamed Neutron network. Mutation testing surfaced it. There's an example pinning it now, so the guard is tested rather than merely harmless.

The suite was checked by mutation testing

Since the theme of the review was tests that pass without proving anything, coverage alone didn't seem like enough. 40 deliberate mutations were applied to lib/ and the suite re-run against each: 38 killed, 2 provably equivalent (return if merged.empty?, whose removal can't change behaviour because an empty merged implies an empty cc; and the guard above, which was then made distinguishable and is now killed too).

That also turned up something the fog_doubles change had hidden. Making wait_for actually run its block took the suite from 0.31s to 12.4s: create's block calls sleep(1), and under instance_exec that's Kernel#sleep on the server, not the driver — so the driver's sleep stub never applied to it. The "nothing sleeps" property was previously true only because the block was dead code. Stubbed on the right object now, back to 0.31s.

Left for a follow-up

The ingest-side coercion layer in clouds.rb, taking your "follow-up candidate rather than a blocker" framing. Both halves of the drift reproduce — identity_api_version: 3 is "3" in config (what kitchen diagnose prints) but "v3" at the Fog boundary, and openstack_tenant/openstack_tenant_id are in FOG_STRING_SETTINGS but not STRING_CONFIG_KEYS. Consolidating changes diagnose output, which is a separable behaviour change, and this PR is already carrying three breaking ones. That thread is the one I left unresolved.

Also still untouched by agreement: no_ssh_tcp_check, no_ssh_tcp_check_sleep and pre_create_command, which you're picking up separately.

Verification

$ bundle exec rake coverage && bundle exec rake style
360 examples, 0 failures
Line Coverage: 100.0% (474 / 474)
Branch Coverage: 97.19% (173 / 178)
11 files inspected, no offenses detected

$ bundle exec yard stats
100.00% documented

Order-independent across 5 seeds, and verified clean against a planted ~/.config/openstack/secure.yaml with OS_CLOUD/OS_USERNAME/OS_CACERT exported.

🤖 Generated with Claude Code