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.
Add restore-memory step injection for custom jobs #44037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Add restore-memory step injection for custom jobs #44037
Changes from 2 commits
082dd7b1261f3d1350e5875e0efd080e977eb86873e41323174a4e181fe026cf05f676d694429800c01cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] Non-bool values for
cache-memory,repo-memory, orcomment-memoryare silently dropped — there's no error returned when a user writescache-memory: 1orcache-memory: yes(which YAML parsers may resolve to non-bool types). The value is simply ignored, leading to confusing behaviour where the requested restore never appears.Suggestion
Return an error on type mismatch rather than silently ignoring the value:
Add a test case such as
cache-memory: 1toTestExtractRestoreMemoryConfigto document the expectation.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent no-op for non-bool memory values — should return a validation error
If the user writes
cache-memory: "true"or any non-boolean YAML value, thev.(bool)type assertion fails silently and the field is ignored (leftfalse). The schema says"type": "boolean"so the YAML parser may already reject this, but at the Go level this pattern allows a misconfigured workflow to compile without any indication that the requested memory restore was skipped.Consider returning an error for unexpected non-bool values to give the user an actionable message:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superseded by the API simplification:
restore-memorynow accepts only a top-levelbool. TheextractRestoreMemoryConfigfunction already returnsfmt.Errorf("jobs.%s.restore-memory must be a boolean (true or false)", jobName)for any non-boolvalue. The per-typermMapno longer exists, so silent no-ops for sub-keys are not possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs] Silent no-op when
resolveActionReferencereturns""andactionMode.IsScript()is false —setupLinesis empty butmemoryLinesforrepo-memory/comment-memoryare still emitted, causing the injected steps to reference scripts (clone_repo_memory_branch.sh,setup_comment_memory_files.cjs) that were never installed.Suggestion
Either surface an error or short-circuit all memory lines when setup cannot be satisfied:
A unit test covering this code path would prevent silent regressions.
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e413231: raised to a compile-time error. When
resolveActionReference("./actions/setup", data)returns""and!c.actionMode.IsScript(),buildRestoreMemoryStepsnow returns an error message identifying the job and the requirement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent skip of setup step leaves repo-memory/comment-memory broken at runtime
When
setupActionRef == ""and!c.actionMode.IsScript(), theifguard exits without appending any setup lines — but thegenerateRepoMemoryRestoreLines/generateCommentMemoryRestoreLinessteps are still emitted unconditionally in the block below. Both of those steps call scripts installed by the setup action (clone_repo_memory_branch.sh,setup_comment_memory_files.cjs). If the setup action is not injected, those scripts will not exist on the runner and the steps will fail at runtime.Other callers (e.g.
compiler_safe_outputs_job.go:124) follow the same guard and also skip their memory steps when the setup action is absent, or they log the situation. This site should either (a) also skip the memory lines, or (b) treat the missing setup action as a compilation error:@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e413231:
buildRestoreMemoryStepsnow returns a compile-time error whenresolveActionReferencereturns""and!c.actionMode.IsScript(). The method signature was updated to(setupLines, memoryLines []string, err error)and the call site propagates the error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ADR (docs/adr/44037-restore-memory-read-only-access-custom-jobs.md) now shows the correct canonical paths (
/tmp/gh-aw/cache-memoryfor the default cache,/tmp/gh-aw/cache-memory-<id>for named caches) and the updatedrestore-memory: trueboolean API (committed in 1fe026c). The PR description is stale from the original code generation and reflects the pre-simplification per-type map API; the ADR is the authoritative documentation going forward.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design] The restore-key construction logic here is a near-duplicate of the same logic in
cache.go(lines ~529-560). Any future change to restore-key semantics (e.g. adding a new scope type) must now be applied in two places.Suggestion
Extract the restore-key derivation into a shared helper, for example
buildCacheRestoreKeys(cacheKey string, scope string) []string, and call it from bothgenerateCacheMemoryRestoreLinesand the existinggenerateCacheMemoryStepsincache.go.This removes the duplication and makes the parallel structure explicit.
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e413231: extracted
buildCacheRestoreKeys(cacheKey, scope string) []stringintocache.goand replaced both the inline logic ingenerateCacheMemorySteps(cache.go) and the duplicate ingenerateCacheMemoryRestoreLines(compiler_custom_job_memory.go) with calls to this shared helper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
generateRepoMemoryRestoreLinessplits astrings.Builderoutput on"\n"and re-appends"\n"to every part, which adds a spurious trailing"\n"for the last element (the finalstrings.Splitresult for"...\n"will have an empty string at the end, becoming"\n"). This produces a blank extra line appended to the step block.Suggestion
Or test that the generated YAML for a
repo-memorycustom job has no duplicate blank lines.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e413231:
generateRepoMemoryRestoreLinesnow usesstrings.SplitAfter(raw, "\n")and skips empty elements, so no phantom blank line is injected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: double trailing newline in
generateRepoMemoryRestoreLinesstrings.Split(raw, "\n")on a newline-terminated string always produces an empty string as the last element. Adding"\n"to every part then emits an extra blank"\n"line at the end, injecting a spurious empty line between the repo-memory steps and whatever follows (pre-steps, regular steps).Fix: skip the trailing empty element, or use
strings.SplitAfterwhich preserves newlines without adding a phantom one:@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e413231:
generateRepoMemoryRestoreLinesnow usesstrings.SplitAfter(raw, "\n")and skips empty parts, eliminating the spurious trailing blank line.Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.