chore: remove unneccessary WithFailFast option #4484
Merged
Conversation
This file contains 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
WithFailFast was not actually causing any change in behavior, so we can remove the option completely. The option set FailOnNonTempDialError(true) on the gRPC connection. However, the help text for this method option declares that it "does not do anything useful unless you are also using WithBlock()" - which we do not do. I've verified that the only client-side code path that actually handles this option is under a check for if WithBlock() was also used. We can remove this option instead of fixing it, since the need for this functionality has been replaced by the more buildkit-native client.Wait function - this function also correctly waits for the buildkit server to begin running, and to start serving requests (which is generally the desired behaviour). If users actually desire this option (which was not fully working previously), they can use the WithGRPCDialOption, and pass FailOnNonTempDialError(true) directly, alongside WithBlock. Signed-off-by: Justin Chadwell <me@jedevc.com>
|
Note: this will cause a little bit of breakage for anyone who was a custom buildkit client, but given that this option doesn't seem to have done anything for a while, I'm happy with the idea of removing it. |
tonistiigi
approved these changes
Dec 13, 2023
AkihiroSuda
approved these changes
Dec 14, 2023
kzys
added a commit
to superfly/flyctl
that referenced
this pull request
Mar 29, 2024
It is no-op and has been removed from Buildkit. moby/buildkit#4484
kzys
added a commit
to superfly/flyctl
that referenced
this pull request
Mar 29, 2024
It is no-op and has been removed from Buildkit. moby/buildkit#4484
|
Hey @jedevc - @AkihiroSuda sent me here.
Just using a normal BuildKit client, not a custom one as far as I'm aware. A deprecation would have been much appreciated here. I did a code search but didn't find this PR until I had to ask a maintainer or otherwise bother you guys with an issue asking about it 🙏 Thanks for all your work on BuildKit Alex |
kzys
added a commit
to superfly/flyctl
that referenced
this pull request
Apr 5, 2024
* refactor: don't use WithFailFast It is no-op and has been removed from Buildkit. moby/buildkit#4484 * chore: upgrade moby/buildkit DisplaySolveStatus has been removed by moby/buildkit#4113.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WithFailFastwas not actually causing any change in behavior, so we can remove the option completely.The option set
FailOnNonTempDialError(true)on the gRPC connection. However, the help text for this method option declares that it "does not do anything useful unless you are also usingWithBlock()" - which we do not do. I've verified that the only client-side code path that actually handles this option is under a check for ifWithBlock()was also used.We can remove this option instead of fixing it, since the need for this functionality has been replaced by the more buildkit-native client.Wait function - this function also correctly waits for the buildkit server to begin running, and to start serving requests (which is generally the desired behaviour).
If users actually desire this option (which was not fully working previously), they can use the
WithGRPCDialOption, and passFailOnNonTempDialError(true)directly, alongsideWithBlock.