Repository navigation
fix: repair the generated converge scripts so services actually load - #97
Merged
Merged
Conversation
Three things in the scripts this provisioner generates were wrong in ways that a string-equality spec written from the same typo could never catch. The `hab svc load` wait loop never timed out. The guard was spelled `[$timer -gt 300]` with no spaces, which bash parses as a command named `[0`, and the counter was incremented with `$timer++`, which bash parses as a command named `0++`. Both were "command not found" on every iteration, so `service_load_timeout` did nothing and a service that failed to start hung the converge until Test Kitchen was killed. It is now `[ "$timer" -ge N ]` and `timer=$((timer + 1))`, and it prints what it gave up waiting for before exiting non-zero. The hab user and group were created in the wrong order with the wrong checks. `id -u hab` tested for the user and then ran `groupadd`, while `id -g hab` tested for a group in a way that only works once the user already exists and then ran `useradd -g hab`. A stray `id -u hab || useradd hab` at the top of the script created the user without the hab group first, which made both blocks no-ops. The group is now created first with `getent group hab`, then the user is added to it. An unset `depot_url` was written into the systemd unit as `Environment="HAB_BLDR_URL="`. An empty value is not the same as an unset one -- it overrides the hab CLI's own default with a URL it cannot parse. The `HAB_BLDR_URL` and `HAB_LICENSE` lines are now emitted only when those options are actually configured. Separately, `install_latest_artifact` with no matching .hart in the results directory fell through to `File.basename(nil)` and raised "no implicit conversion of nil into String", which says nothing about what went wrong. It now raises a UserError naming the glob it looked for and the directory it looked in. Specs cover each of these, and the generated Linux scripts are now handed to `bash -n` so a future typo in a heredoc fails the build rather than being copied into an expectation. 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.
I ran the shell this provisioner generates through
bash -n, then actually converged it against a real machine (see #99), and found four things broken in ways our string-equality specs could never catch — the expectations were written from the same typos as the code.A package with a
runfile instead of ahooks/runnever loadedThis is the bad one.
run_commanddecided whether tohab svc loadlike this:The supervisor accepts a run hook in either of two places:
hooks/run, written from a hook template, or arunfile in the package root, which is whatpkg_svc_runin a plan produces. We only ever checked the first.core/redis— the example in our own README — is built the second way:So the converge installed it, skipped the entire load branch, and exited 0. Test Kitchen reported success,
hab svc statussaidNo services loaded, and nothing in the output suggested anything had gone wrong. Both platform paths now resolve the package path once and check both locations; the Windows path also checks the.ps1form of each.The service load loop never timed out
[$timer -gt 300]has no spaces, so bash parses it as a command named[0.$timer++expands to0++, another command name. Both are "command not found" every iteration:service_load_timeoutdid nothing, the counter never moved, and a package that failed to start hung the converge until someone killed Test Kitchen. It is now a real test expression and real shell arithmetic, and it says what it gave up waiting for before exiting non-zero.The hab user and group were created in the wrong order
id -u habtested for the user and then rangroupadd;id -g habtested for a group in a way that only works once the user exists, then ranuseradd -g hab hab. And a strayid -u hab >/dev/null 2>&1 || sudo -E useradd habat the top of the script created the user with its own private group before either block ran, so in practice both were no-ops and the hab group was never created. The group is now created first withgetent group hab, and the user is added to it.An unset
depot_urlwrote a blank Builder URL into the unit fileThe systemd unit always got both environment lines, so leaving
depot_urlalone producedEnvironment="HAB_BLDR_URL=". Empty is not the same as unset — it overrides thehabCLI's own default fromcli.tomlwith a URL it cannot parse.HAB_BLDR_URLandHAB_LICENSEare now written only when configured.And
install_latest_artifactfailed with a useless messageWith no matching
.hart,Dir.glob(...).max_byreturns nil and we calledFile.basename(nil), so the user gotno implicit conversion of nil into String. There was a# TODO: throw error and bail if there's no artifactssitting right above it. It now raises aUserErrornaming the glob and the directory it searched.Tests
Each fix has a spec. I also added a small
bash -nhelper and pointed it atinstall_command,init_command,prepare_commandandrun_command, so the next typo in one of those heredocs fails the build instead of being copied into an expectation.(81.82% / 68.85% before.)