Make the generated action references configurable#535
Open
lahma wants to merge 3 commits into
Open
Conversation
The generator's cache pin sat at v4 while Fallout-build#533 moved checkout, setup-dotnet, and upload-artifact to their current majors — cache was never in that PR's scope. v6.1.0 is current; v4 is two majors behind. - v5.0.0 moved to the node24 runtime and requires a self-hosted runner >= 2.327.1 - v6.0.0 updated dependencies and migrated to ESM The `path:` and `key:` inputs the generator emits are unchanged across both, so the emitted step needs no other edits. Covers the generated workflows, the hand-written publish-packages-* ones, the 14 Verify snapshots with a cache step, and the docs sample. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vn8JQZRwdoovPuctD21Wjx
The generator hard-coded the four actions it emits, so a consumer who wanted a newer major — or a SHA pin — had to subclass GitHubActionsAttribute and reimplement step emission. Two repos migrating off NUKE carried custom step classes for exactly that (Fallout-build#533). Each emitting step now carries a normalizing property, forwarded from four new attribute properties: CheckoutAction, CacheAction, SetupDotNetAction, UploadArtifactAction A value without a '/' is a bare ref appended to the default action name, so "v8" emits actions/checkout@v8. Anything else is a complete reference emitted verbatim — a fork, or a commit pin whose version rides along as a trailing YAML comment. Both forms take that comment, which is what makes the SHA-pinning idiom readable. Null or whitespace restores the default. GitHubActionsDefaults exposes every pinned version as a const — name, version, and the composed reference — so the values are referenceable from an attribute argument and there is one place to bump them. Resolution lives in the step setters rather than the attribute, so subclasses and hand-built steps get the same shorthand and validation. Additive: with no property set the emitted YAML is byte-identical, which the untouched Verify snapshots assert. Closes Fallout-build#534 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vn8JQZRwdoovPuctD21Wjx
Review follow-up on the configurable action references.
Resolution was classifying the override with a bare Contains('/') over the
whole string, so a slash anywhere — in a branch ref like releases/v1, or in a
URL inside the trailing comment — misread the value and then failed the
ref-presence check. That check was itself a substring Contains('@'), which an
'@' in a comment satisfied, letting a reference with no version through to the
workflow file. And with only a CR/LF guard, a value carrying ': ' was emitted
into the unquoted uses: scalar and corrupted the whole document.
Resolution now splits the trailing comment off first and classifies on what
remains: an '@' means a complete reference, otherwise it is a bare ref. A ref
containing a '/' reads exactly like an owner/repo, so it takes a leading '@' to
disambiguate — '@releases/v1' — and the ambiguous form is rejected with a
message naming both fixes rather than guessed at. The reference token must
carry no whitespace, which is what keeps a stray key out of the YAML.
The four overrides are now resolved up-front in GetConfiguration. Cache and
artifact steps are conditional, so their overrides were previously unvalidated
and silently ignored whenever caching or publishing was off — the typo only
surfaced later, attributed to the unrelated edit that re-enabled the step.
Errors name the property and the workflow.
GitHubActionsDefaults is internal, and keeps only the four composed constants.
The properties take a plain string, so nothing a consumer writes needs the
type; the split *Name/*Version constants had no callers outside XML docs. This
also stops the transition-shim generator mirroring it into Nuke.* as an empty
class — it forwards static methods, never consts — which would have given
mid-migration consumers CS0117 on every member, and CS0104 on the bare name.
A custom step's Uses is emitted verbatim and has no default to resolve against,
so it now must name its action in full. Without that, the shorthand documented
for the built-in steps silently produced an unusable 'uses: v8', and a
multi-line value could inject arbitrary keys into the generated workflow.
Also makes the local/container prefix checks ordinal — as a culture-sensitive
comparison the guard's outcome depended on the machine's globalization mode.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vn8JQZRwdoovPuctD21Wjx
ITaluone
approved these changes
Jul 24, 2026
ITaluone
left a comment
ITaluone
left a comment
Collaborator
There was a problem hiding this comment.
In general: LGTM
Just this one question, tho
| // 'releases/v1' is a branch rather than an owner/repo. | ||
| var isExplicitBareRef = reference.StartsWith("@", StringComparison.Ordinal); | ||
| if (isExplicitBareRef) | ||
| reference = reference.Substring(startIndex: 1); |
Collaborator
There was a problem hiding this comment.
❓(non-blocking): isn't this a little bit too implicit? Given, that the XML summary on the Resolve() method never gets to the actual user (since the class is internal)
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
2 participants
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.
The generator hard-coded the four actions it emits, so a consumer who wants a newer major — or a SHA pin — has to subclass
GitHubActionsAttributeand reimplement step emission. #533 bumped three of them; this makes all four overridable so lagging a release behind never blocks anyone.Closes #534.
Attribute properties
CheckoutAction,CacheAction,SetupDotNetAction,UploadArtifactAction.A value containing an
@is a complete reference, emitted as-is; anything else is a bare ref appended to the default action:The trailing
# commentis split off before the value is classified, so a slash or an@inside it changes nothing — that's what makes the SHA-pinning idiom work. A ref that itself contains a/(a branch likereleases/v1) reads exactly like anowner/repo, so it takes a leading@—"@releases/v1"— and the ambiguous bare form is rejected rather than guessed at. Null or whitespace restores the default.The reference token must carry no whitespace: emitted into an unquoted
uses:scalar, a stray:would otherwise corrupt the whole workflow file. All four are resolved up-front inGetConfiguration, so an override is validated even when its step isn't emitted (caching off, artifacts disabled) — errors name the property and the workflow.Resolution lives in the step setters, so subclasses and hand-built steps get the same shorthand and validation.
GitHubActionsCustomStep.Useshas no default to resolve against, so it now must name its action in full instead of silently emitting an unusableuses: v8.GitHubActionsDefaultsis internal — the properties take a plain string, so nothing a consumer writes needs the type.actions/cachev4 → v6 (first commit)Cache was never in #533's scope and sat two majors behind. Separate commit; the diff touches only
actions/cache@v4→@v6lines.The
path:/key:inputs are unchanged. Consumers on older self-hosted runners setCacheAction = "v4"— the feature demonstrating itself.Verification
dotnet build fallout.slnxclean;dotnet test tests/Fallout.Common.Specs— 168 passed, 0 failedaction-overridescovers all four properties in one workflow, one per accepted formGitHubActionsActionReferenceSpecscovers the resolver: fallback, bare ref,@-prefixed slash-bearing ref, SHA + comment, comment containing a URL, complete reference, fork, local/container refs, the ambiguous form, comment-only, and YAML-corrupting values — plus validation of a skipped step's overrideGitHubActionsDefaultsintoNuke.*as an empty class (it forwards static methods, never consts)CheckoutAction = "v8"and aCacheActionoverride on this repo'sbuildworkflow, regenerated, confirmed both lines changed, reverted — regeneration with no overrides leaves.github/workflowsuntouchedNot in scope
ITaluone's original "heads-up when a new action release ships" — worth a follow-up issue; this removes the urgency but not the need.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Vn8JQZRwdoovPuctD21Wjx