Allow extra refs to be marked as optional in run-task#935
Open
Eijebong wants to merge 1 commit intotaskcluster:mainfrom
Open
Allow extra refs to be marked as optional in run-task#935Eijebong wants to merge 1 commit intotaskcluster:mainfrom
Eijebong wants to merge 1 commit intotaskcluster:mainfrom
Conversation
jcristau
reviewed
Apr 15, 2026
Eijebong
force-pushed
the
make-notes-optional
branch
from
4d26abc to
b644e55
Compare
jcristau
reviewed
Apr 15, 2026
Comment on lines
+763
to
767
| for ref in extra_refs or []: | ||
| optional = ref.startswith("?") | ||
| if optional: | ||
| ref = ref[1:] | ||
| git_fetch( |
Contributor
There was a problem hiding this comment.
Can we avoid calling git_fetch once per ref?
Contributor
Author
There was a problem hiding this comment.
I don't think so? Cause we cannot extract what ref failed to fetch, so we'd have to check them all after the fact
Eijebong
force-pushed
the
make-notes-optional
branch
from
b644e55 to
0c9f3b1
Compare
This prevents failures if a fork doesn't have any notes at `refs/notes/decision-parameters`. I chose to pay for the `ls-remote` time if an optional ref failed to fetch to avoid having to parse the stderr from git fetch which felt very fragile. It adds a potential ~1.5s to fetches per optional remote that is missing which isn't ideal, but I don't have anything better at this point in time. The new log message is slightly misleading since technically ls-remote might find one ref but not another and it'd be reported that both are missing but I don't want to have to ls-remote for each ref, adding 1.5s each time, to fix this.
Eijebong
force-pushed
the
make-notes-optional
branch
from
0c9f3b1 to
7031852
Compare
Contributor
Author
|
(I dropped the taskcluster.yml change and pushed empty notes to my fork to get CI green, I'll re-add the |
ahal
reviewed
Apr 15, 2026
|
|
||
| if extra_refs: | ||
| for ref in extra_refs or []: | ||
| optional = ref.startswith("?") |
Collaborator
There was a problem hiding this comment.
Tbh, I think I'd rather these just never fail, and if there's some use case where we absolutely need the extra ref fetched, then we can deal with it then
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
3 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.
This will help preventing failures if a fork doesn't have any notes at
refs/notes/decision-parameters. I chose to pay for thels-remotetime if an optional ref failed to fetch to avoid having to parse the stderr from git fetch which felt very fragile. It adds a potential ~1.5s to fetches per optional remote that is missing which isn't ideal, but I don't have anything better at this point in time. The new log message is slightly misleading since technically ls-remote might find one ref but not another and it'd be reported that both are missing but I don't want to have to ls-remote for each ref, adding 1.5s each time, to fix this.