Repository navigation
Conversation
FakeShellOut, which shell_out returns when running over a train transport connection (chef target mode), built its status with OpenStruct but never required ostruct. It only worked when something else in the process had already loaded it; otherwise it raised NameError. ostruct is also no longer a default gem as of Ruby 3.5, so requiring it would just move the failure to consumers whose Gemfile doesn't list it. Replace it with a small Struct that answers success?. Also teach cspell the words helper.rb already uses, since the spellcheck job only scans files a PR touches. Signed-off-by: Tim Smith <tsmith84@proton.me>
Spec infrastructure: - Modern RSpec defaults: RSpec.describe without monkey patching, random ordering, verified partial doubles, --only-failures support. - Fail the run if lib/ emits a Ruby warning. - Drop the DependencyProc ruby-version filter; the gemspec requires 3.1+. - Tag the cgroup specs :requires_root and skip explicitly when cgroup v2 is missing, instead of passing without asserting anything. New coverage: - Helper: the shell_out_compacted argument contract chef's own specs stub against, default locale/PATH injection and overrides, default_env: false, chef provider timeouts, live streaming, and the train transport path (command joining, cwd/input wrapping, FakeShellOut results). - ShellOut: array commands bypass the shell, env reaches the child without leaking into the parent, umask, signal-killed children, sensitive output kept out of exceptions, logger/log_tag/log_level, elevated rejected on unix. - Root-only: uid/gid switching and login simulation, including dropping root's supplementary groups. - Packaging: VERSION matches version.rb, both gemspecs are valid and ship the right files and dependencies, the library loads cleanly with frozen string literals and warnings on, and requiring it does not pull in tmpdir/fileutils (chef#282). Signed-off-by: Tim Smith <tsmith84@proton.me>
Replace the Buildkite verify pipeline with a GitHub Actions workflow: - Linux, macOS and Windows on every supported Ruby (3.1 through 4.0), plus ruby-head as an allowed failure. - The full suite as root on Linux, so user/group switching, login simulation and cgroup specs actually run. - A run with --enable-frozen-string-literal forced on for everything. - Build both gems with --strict, install the result in an isolated GEM_HOME and smoke test it outside the source tree. Pin every action to a commit SHA with a version comment and add dependabot (with a cooldown) for actions and bundler. Update actions that had fallen behind or tracked a branch. Skip the DCO check on dependabot PRs, since dependabot cannot sign off. Give the gemspec a real description so gem build --strict passes, and drop Gemfile branches for Ruby 3.0, which is no longer supported. Signed-off-by: Tim Smith <tsmith84@proton.me>
Swap the Buildkite badge for GitHub Actions, use shields.io for the gem version and add a license badge. Link docs instead of bare URLs, point the "see also" section at Process.spawn/Open3, use https for the license URL, and document how to run the specs, including the root-only ones. Fix the CONTRIBUTING link that still pointed at master, and update the copilot instructions for the new CI layout. Signed-off-by: Tim Smith <tsmith84@proton.me>
With login: true and a user but no explicit group, #gid resolves the user's primary group, but set_group only acted when group was set. The child switched uid and cleared supplementary groups but kept the parent's primary group, so a command run as "nobody" from a root process still had gid 0. Check #gid instead. Without login and without a group #gid is still nil, so that case is unchanged. Signed-off-by: Tim Smith <tsmith84@proton.me>
wmi-lite requires win32ole, which is a bundled rather than default gem as of Ruby 4.0, so it can no longer be required under bundler unless it is declared. mixlib-shellout loads wmi-lite when a command times out, to kill the process tree, so on Ruby 4.0 every timeout raised LoadError instead of CommandTimeout and left the process tree running. Add it to the Windows gem's dependencies and the Gemfile. Signed-off-by: Tim Smith <tsmith84@proton.me>
- Run the killed-by-signal spec without a shell. dash, /bin/sh on Debian/Ubuntu, forks instead of exec'ing, so it survived and reported 137 rather than being the killed process. - Use real doubles in the kill_process_tree specs; stubbing methods that bare Objects don't have fails with verified partial doubles. - Skip -w in the load spec on Windows, where core_ext.rb deliberately redefines win32-process's Process.create. Signed-off-by: Tim Smith <tsmith84@proton.me>
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
1 participant
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.
Summary
Moves testing to GitHub Actions and fills the gaps in the spec suite that could let a regression through unnoticed. The new specs found three real bugs; each is fixed in its own commit.
Bug fix:
FakeShellOutraisedNameErrorThe helper returns
FakeShellOutwhen commands run over a train transport (chef target mode). It usedOpenStructbut never requiredostruct, so it only worked if something else had loaded it first.ostructis also no longer a default gem as of Ruby 3.5. It's now a smallStruct. Without the fix, 8 of the new helper specs fail with exactly thatNameError.Bug fix:
login: truekept the parent's primary groupWith
user:andlogin: truebut nogroup:,#gidresolves the user's primary group, butset_grouponly ran whengroupwas set. The child switched uid and cleared supplementary groups, yet kept the parent's primary gid. Run from root, a command "as nobody" still had gid 0. Found by the new root-only CI job.Bug fix: timeouts raised
LoadErroron Windows with Ruby 4.0wmi-literequireswin32ole, which is a bundled rather than default gem as of Ruby 4.0.mixlib-shelloutloadswmi-liteon timeout to kill the process tree, so every timeout raisedLoadError: cannot load such file -- win32oleinstead ofCommandTimeout, and the process tree kept running.win32oleis now a dependency of the ucrt gem and in the Gemfile on Windows.CI (
.github/workflows/ci.yml)ruby-headas an allowed failure.--enable-frozen-string-literalforced on for everything.--strict, installs the.geminto an isolatedGEM_HOME, and smoke tests it outside the source tree.Specs (147 → 198 examples locally)
shell_out_compactedargument contract that Chef's own specs stub against, default locale/PATH injection and overrides,default_env: false, Chef provider timeout injection, live streaming, and the full train transport path.nilexitstatus and count as errorssensitive:keeps output out of exception messageslog_tagandlog_levelelevated:is rejected on UnixVERSIONmatchesversion.rb(Expeditor's sed bump)tmpdir/fileutils(regression guard for Don't load tmpdir and fileutils at require time #282; verified to fail against pre-Don't load tmpdir and fileutils at require time #282 code)--only-failureslib/fails the runActions and dependabot
actionshub/dcoandget-pr-commitswere tracking@main, andlinelintwas tracking@master..github/dependabot.ymlupdates actions and bundler weekly, grouped, with a 7-day cooldown.README
master.Needs a maintainer
.expeditor/verify.pipeline.yml, its runner scripts, and theverifypipeline entry. Expeditor's version bump and publish steps are untouched. If branch protection requires the Buildkiteverifycheck, it needs to be swapped for the new CI checks.ci-main-pull-request-stub-1.0.7.ymlis managed centrally and left as is. It still uses the deprecated::set-output.