[Superseded by #31] refactor: internal cleanups after the parameter-surface restructure#30
Closed
jcardozo-eth wants to merge 12 commits into
Closed
Conversation
The socket-derivation comment claimed an "exotic external socket is expressible via runner_service.environment", which overstates the reach. That environment only sets the manager service drop-in's DOCKER_HOST; the socket_mount volume and the healthcheck probe keep using the uid-derived path. Reword to state the escape hatch narrowly so it is not read as a general external-daemon switch.
Add a non-rendered EPP comment at the top of the healthcheck template recording that the manager-service check asserts the packaged gitlab-runner systemd unit (systemctl is-active gitlab-runner), the name the package defines rather than a module-derived value. Source-only, so the rendered script and its golden are unchanged.
The manager's systemd unit name ('gitlab-runner') was a literal in
service.pp and again in the healthcheck template. Derive it once as
$service_name in init.pp and thread it into both. Value unchanged, so
the compiled catalog and the healthcheck golden are byte-identical.
config.pp built the ~/.config/systemd/user drop-in parent chain top-down with dirname(), and rootless_docker.pp hardcoded the same path for the setuptool's generated docker.service. Derive the chain once in init.pp, built bottom-up (each directory its parent plus one element), and share $user_systemd_dir with both. Paths are unchanged, so the compiled catalog is byte-identical.
The apply template built the Puppetfile path inline from the control repository path, twice, while its siblings (manifest, module directory, Hiera config) arrive pre-derived from the manifest. Derive puppetfile_path in self_update.pp and pass it in, and record the Puppetfile in the control_repository_path option doc. Rendered script and catalog unchanged.
The $real_service_environment selector's inner `undef => []` arm looks like it could render an empty Environment. Note that it is reached only when runner_service.manage is off (the guard above rejects that case while the service is managed), so [] is the never-consumed value that keeps the selector total, not a rendered empty Environment.
The runner_service.environment option was documented as Array[String] while the struct enforces Array[Pattern[/\A[^\r\n]+\z/]], and standalone.self_update as a bare Hash while the struct spells out manage/apply_interval/apply_timeout. Match the doc types to the enforced types so REFERENCE states the real contract.
The struct_param helper mirrors the module's deep-merge lookup_options so resource-style overrides merge over the module defaults the way a Hiera consumer's partial hash would. The hand-rolled recursive merge replaced whole arrays; Hiera's deep strategy unions them and applies the '--' knockout prefix. Use the deep_merge gem that strategy is built on so the helper matches Hiera, deep-copying both operands since deep_merge! mutates its destination.
rendered_config duplicated rendered_file's Sensitive-unwrap logic and
carried a path parameter no caller passed; define it in terms of
rendered_file at the fixed default path. Collapse the repeated
struct_param('runner_account', 'uid' => N) boilerplate into one
account_with_uid helper. Test-only; the suite is unchanged.
The account name and home were re-subscripted out of runner_account in five files (config, user, service, self_update, rootless_docker). Hoist $runner_name and $runner_home once in init.pp, beside the existing $runner_group, and have each concern read the local. Values unchanged, so the compiled catalog is byte-identical.
The docker and gitlab_runner sources under packages.sources declared the
same inline Struct[{ location, key_source }] twice in the class
signature. Extract it as Rootless_gitlab_runner::Apt_source and reference
the alias in both. Same type constraint, so the compiled catalog is
byte-identical; REFERENCE regenerates with the alias.
packages.sources was documented as a bare Hash while the struct enforces manage plus the docker and gitlab_runner apt sources. Spell the contract out in the option type, referencing the Apt_source alias, so REFERENCE states the real shape like the other options.
Member
Author
jcardozo-eth
changed the title
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.
Changes
A set of behavior-neutral internal cleanups, one concern per commit, on top of the
runner_account.groupwork.Repeated literals become shared derivations, each computed once and consumed by the concerns that need it: the manager service unit name, the systemd user unit directory (now built bottom-up, child from parent, instead of walking back up with
dirname()), the Puppetfile path (derived beside the other apply paths), and the runner account identity —nameandhomehoisted besidegroupso the private classes read locals instead of re-subscriptingrunner_accountper file. The apt-source sub-struct becomes a named type alias in place of two identical inline copies.Documentation is brought in line with the code: the
@optiondoc types match the enforced structs (runner_service.environment's newline-rejecting pattern,standalone.self_update's subkey contract, andpackages.sources' apt-source shape), thecontrol_repository_pathoption records thePuppetfile, the socket escape-hatch and selector-totality comments are scoped to what they actually cover, and the healthcheck template records the packaged unit its manager-service check asserts.Test-only changes:
struct_paramnow merges through thedeep_mergegem that Hiera's deep strategy is built on, so a resource-style override unions arrays and honors the--knockout prefix the way a Hiera consumer's partial hash would; and therendered_configandrunner_account-uid setup helpers are deduplicated.Verification
puppet-lint,metadata-json-lint,yamllint, and theREFERENCE.mddocs check.