Repository navigation
fix: clear destroyed servers from state and stop masking create failures - #45
Merged
tas50 merged 1 commit intoAug 30, 2026
Merged
Conversation
A handful of related problems in the create/destroy lifecycle. `destroy` never removed `server_id` and `hostname` from the state hash. Test Kitchen writes state back in an `ensure` block even when an action fails, so when `wait_for_server` gives up and tears the machine down, the state file is still left naming a server that no longer exists. The next `kitchen destroy` then runs the destroy workflow a second time against a machine that is already gone. `destroy` now clears both keys once the workflow succeeds, which also makes it a no-op the second time it is called in a process. `wait_for_server` replaced the connection failure with whatever the cleanup raised. If the machine was unreachable *and* the destroy workflow then failed, the user saw the vRO error and no hint of why the machine was being destroyed in the first place. The cleanup failure is now logged -- including a warning that the server may have been left behind -- and the original connection failure is what propagates. `set_workflow_vars` cleared the memoized client but not the memoized output parameters, so anything reading `output_parameters` after switching from the create workflow to the destroy workflow would still be looking at the create workflow's output. `output_parameter_value` raised `NoMethodError` on nil for any parameter the workflow did not return, which made `output_parameter_empty?` blow up instead of answering the question it was asked. It now returns an empty string for a missing parameter. That in turn removes a dead branch: `output_parameter_empty?` tested `.nil?` on the result of a `.to_s`, which can never be nil, and computed the value twice to do it. Also requires `timeout` explicitly rather than relying on a dependency having pulled it in, and updates a `rubocop:disable` comment that still used the old `Style/AccessorMethodName` namespace -- cookstyle printed a deprecation warning about it on every run. Signed-off-by: Tim Smith <tim@mondoo.com>
tas50
force-pushed
the
fix/state-cleanup-and-error-handling
branch
from
August 30, 2026 01:54
ff073dd to
6b0be2d
Compare
tas50
added a commit
that referenced
this pull request
Aug 30, 2026
* test: modernize the RSpec suite and cover the workflow logic The spec_helper was an empty license header: no requires, no RSpec configuration, and nothing in `.rspec` to load it. Every spec file had to require its own dependencies, and none of the guardrails that catch stale tests were switched on. This wires the suite up properly: - `.rspec` now passes `--require spec_helper`, so the helper is the single place that loads the driver and its Test Kitchen dummies. - `spec_helper` turns on `verify_partial_doubles`, `disable_monkey_patching!`, expect-only syntax, `raise_errors_for_deprecations!`, and random ordering with a printed seed. - `spec/support/vro_doubles.rb` builds verifying doubles for the slice of the vcoworkflows API the driver touches, so a rename upstream fails the suite instead of passing against a stub that no longer matches. - Specs are no longer excluded from cookstyle, so they are held to the same style as `lib/`. It also fills in the logic the old suite left uncovered. Previously `#create` and `#destroy` were only checked by stubbing the driver's own methods and asserting they were called, which proves nothing about what the driver actually sends to vRO. There are now end-to-end examples that stub only at the `VcoWorkflows::Workflow` boundary and assert the real behaviour: which workflow gets looked up, that configured create and destroy parameters are pushed to it, that `server_id` reaches the destroy workflow, and that the create output lands in instance state. New coverage for the branches where bugs can hide: - the poll loop in `#wait_for_workflow` -- how many times the token is re-read, that it sleeps two seconds between polls, that it does not sleep when the run is already finished, and that a real elapsed timeout fires - `request_timeout` appearing in the timeout message when it is not the default - `#set_workflow_parameters` stringifying symbol keys, passing non-string values through, and no-opping on an empty hash - `#output_parameter_value` stringifying numbers and unset values - `vro_disable_ssl_verify` reaching `VcoWorkflows::Config` - `#execute_create_workflow` not validating or writing state when the run failed - memoization of `vro_config`, `vro_client`, and `output_parameters` Also removes a `Struct.new` assigned to a top-level constant inside an example, which leaked a constant into the whole suite and warned on redefine, and corrects the spec path in CONTRIBUTING.md. 51 examples became 77. Signed-off-by: Tim Smith <tim@mondoo.com> * chore: drop the unused webmock dependency webmock has never been required or used by anything in spec/ -- the specs stub the vcoworkflows client rather than the HTTP layer underneath it, so nothing ever reaches a socket for webmock to intercept. Removing it also clears the one thing standing between this repo and a green lint run: cookstyle 9.0.0 flags the extra spacing in that Gemfile line under Layout/ExtraSpacing, which cookstyle 8 did not, so `main` went red on its own the moment cookstyle 9 shipped. Signed-off-by: Tim Smith <tim@mondoo.com> * fix: clear destroyed servers from state and stop masking create failures (#45) A handful of related problems in the create/destroy lifecycle. `destroy` never removed `server_id` and `hostname` from the state hash. Test Kitchen writes state back in an `ensure` block even when an action fails, so when `wait_for_server` gives up and tears the machine down, the state file is still left naming a server that no longer exists. The next `kitchen destroy` then runs the destroy workflow a second time against a machine that is already gone. `destroy` now clears both keys once the workflow succeeds, which also makes it a no-op the second time it is called in a process. `wait_for_server` replaced the connection failure with whatever the cleanup raised. If the machine was unreachable *and* the destroy workflow then failed, the user saw the vRO error and no hint of why the machine was being destroyed in the first place. The cleanup failure is now logged -- including a warning that the server may have been left behind -- and the original connection failure is what propagates. `set_workflow_vars` cleared the memoized client but not the memoized output parameters, so anything reading `output_parameters` after switching from the create workflow to the destroy workflow would still be looking at the create workflow's output. `output_parameter_value` raised `NoMethodError` on nil for any parameter the workflow did not return, which made `output_parameter_empty?` blow up instead of answering the question it was asked. It now returns an empty string for a missing parameter. That in turn removes a dead branch: `output_parameter_empty?` tested `.nil?` on the result of a `.to_s`, which can never be nil, and computed the value twice to do it. Also requires `timeout` explicitly rather than relying on a dependency having pulled it in, and updates a `rubocop:disable` comment that still used the old `Style/AccessorMethodName` namespace -- cookstyle printed a deprecation warning about it on every run. Signed-off-by: Tim Smith <tim@mondoo.com> --------- Signed-off-by: Tim Smith <tim@mondoo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A handful of related problems in the create/destroy lifecycle.
destroyleft a destroyed server in statedestroynever removedserver_idandhostnamefrom the state hash. Test Kitchen writes state back in anensureblock even when the action failed, so whenwait_for_servergives up and tears the machine down, the state file is still left naming a server that no longer exists. The nextkitchen destroythen runs the destroy workflow a second time against a machine that is already gone — best case a confusing failure, worst case the workflow does something to whatever now holds that id.destroynow clears both keys once the workflow succeeds, which also makes it a no-op the second time it is called in a process. State is deliberately left alone if the destroy workflow itself fails, since the machine is still out there.A failed cleanup hid why the machine was being cleaned up
wait_for_serverrescued the connection failure, calleddestroy, and re-raised. If the destroy workflow also failed, its exception replaced the original — so the user saw a vRO error with no hint that the real problem was that the machine never came up. The cleanup failure is now logged, including a warning that the server may have been left behind, and the connection failure is what propagates.Stale output parameters could leak between workflows
set_workflow_varsclears the memoized client precisely so that running the destroy workflow after the create workflow in the same process is safe. It did not clear the memoized@output_parameters, so anything reading them after the switch would still be looking at the create workflow's output.A missing output parameter raised
NoMethodErroroutput_parameter_valuedidoutput_parameters[key].value.to_s, which explodes on nil for any parameter the workflow did not return — sooutput_parameter_empty?crashed instead of answering the question it was asked. It now returns an empty string for a missing parameter.That also removes a dead branch:
output_parameter_empty?tested.nil?on the result of a.to_s, which can never be nil, and computed the value twice to do it.Housekeeping
require "timeout"explicitly instead of relying on a dependency having pulled it in;Timeout.timeoutis called directly here.rubocop:disablecomment onset_workflow_parametersstill used the oldStyle/AccessorMethodNamenamespace, so cookstyle printed a deprecation warning on every single run. NowNaming/AccessorMethodName.Verification