Cleanup remotefile resolver and delete internal/worker#2178
Open
lzap wants to merge 10 commits intoosbuild:mainfrom
Open
Cleanup remotefile resolver and delete internal/worker#2178lzap wants to merge 10 commits intoosbuild:mainfrom
lzap wants to merge 10 commits intoosbuild:mainfrom
Conversation
The remotefile package provides a content resolver implementation, not a customization. Move it to the pkg/ directory, where also other content resolver packages live. Signed-off-by: Tomáš Hozza <thozza@redhat.com>
Rework the remotefile resolver to not embed the resolution error in the Spec definition, but instead return error from the resolver `Finish()` method. This is consistent with i.e. the container resolver. Additionally, don't use clienterrors package for errors. Signed-off-by: Tomáš Hozza <thozza@redhat.com>
This is a leftover from osbuild-composer split, while the authoritative copy still lives in osbuild-composer repository. The package is not needed in the images library.
The code currently allows for random order of error messages. Update the test to account for this.
To ensure deterministic error messages, sort the error messages before returning them and then remove consecutive duplicate error messages (which are unlikely but the call is cheap).
Previously, the context was completely unused. Let's use it to ensure that possible deadlines are respected.
Previously, the resolver was not concurrent safe. Let's make it so that multiple calls to Add and Finish can be made concurrently.
The resolver is designed in a way that when Add is called after Finish was called, it may panic. Let's document this behavior.
The results are being collected via a buffered channel which has a small size of 2. Collecting of results is very efficient compared to HTTP calls, therefore the chance that the channel will block is very low. While 2 will definitely not hurt, I find it confusing because the name of the field is "queue" which made me think of a queue of items to be processed. That is not the case here, it is queue of results.
These error codes were silently ignored.
thozza
reviewed
Feb 6, 2026
Member
thozza
left a comment
thozza
left a comment
There was a problem hiding this comment.
Thanks for taking this on. All the enhancements make sense, but I don't like the change to make the test cope with indeterminism when the results are actually deterministic.
Comment on lines
+85
to
+87
| assert.Contains(t, err.Error(), "failed to resolve remote files:") | ||
| assert.Contains(t, err.Error(), expectedErrMessageOne) | ||
| assert.Contains(t, err.Error(), expectedErrMessageTwo) |
Member
There was a problem hiding this comment.
I think that it makes more sense to make the output stable (IOW the next commit) and then adjust the test. Making the test account for indeterminism, especially when the error output is made deterministic later is not great.
| func NewResolver(ctx context.Context) *Resolver { | ||
| return &Resolver{ | ||
| queue: make(chan resolveResult, 2), | ||
| queue: make(chan resolveResult), |
Member
There was a problem hiding this comment.
Nitpick: why not rename the field to resultsQueue if the name was confusing to you and made the wrong implications at the first sight?
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.
This builds on top of #1957 and fixes tests. On top of that, it adds a bunch of small improvements which are rather insignificant but nice to have.
This all is needed in order to use the (currently unused) resolver for GPG keys: osbuild/osbuild#2326
Additionally, once this is merged we should make use of it in composer, there seems to be a copy of this functionality:
https://github.com/osbuild/osbuild-composer/tree/main/internal/remotefile
The randomness in tests is solved in two commits, both will work or just one of them too (sort errors or just accept they come in random order).
Context was present but never used.
Then the code was not thread safe, but it is likely only used from one goroutine so it was never a problem. But I think using wait groups is actually nicer code.
What was undocumented was that call of
AddafterFinishwould cause panic, I think it is fair to just document that and live with it. Any solution will make the code less readable than it can be and we do not need that robustness. Honestly, the way this is implemented is very simplistic and could be improved but this is not something we need today.Then I dropped the buffering from the channel, this has no effect and it makes the code a bit confusing. Discuss, I can drop this if you feel strong about this.
Finally, the HTTP error codes were not handled so this would silently create empty files. I am not sure if this was desired behavior, can drop if it was.
I do have some other ideas in mind to further improve this, but I wanted to keep things simple. But just in case:
NewResolver. This usually makes testing easier, setting timeouts or TLS possible.