-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Users/davidmiri/fix docker buildkit warnings 17893 #21650
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
Open
rm-dmiri
wants to merge
5
commits into
master
Choose a base branch
from
users/davidmiri/fix-docker-buildkit-warnings-17893
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Users/davidmiri/fix docker buildkit warnings 17893 #21650
rm-dmiri
wants to merge
5
commits into
master
from
users/davidmiri/fix-docker-buildkit-warnings-17893
Conversation
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
…7893) Problem: - Since Docker 23.0 (Feb 2023), BuildKit is the default builder - BuildKit outputs structured progress to stderr, not stdout - Task incorrectly warned about 'no data written' even when build succeeded - Original fix for #12470 only detected explicit DOCKER_BUILDKIT=1 Solution: - Created isBuildKitExpected() function with smart detection: * Returns true if DOCKER_BUILDKIT=1 (explicit BuildKit) * Returns false if DOCKER_BUILDKIT=0 (explicit legacy) * Returns true if unset (assumes modern Docker 23+ default) - Modified writeTaskOutput() to only warn when legacy builder used - Added debug logging for troubleshooting Evidence: - moby/moby #40379: Docker 23.0 made BuildKit default - BuildKit uses structured progress API, not stdout - User reports confirm issue started March 2023 (Docker 23.0 adoption) Bumped task version to 2.268.0
DockerV1 has the same issue as DockerV2 - false warnings about empty output when BuildKit is in use. Applied identical solution: - Created isBuildKitExpected() function - Only warns when legacy builder is detected - Handles Docker 23+ default BuildKit behavior Bumped task version to 1.268.0
…ComposeV1 All three tasks had the same empty output warning issue. Applied consistent solution across all Docker-related tasks: - DockerV0: Added isBuildKitExpected() and smart warning logic - DockerComposeV0: Added isBuildKitExpected() and smart warning logic - DockerComposeV1: Added isBuildKitExpected() and smart warning logic Version bumps: - DockerV0: 0.265.0 -> 0.268.0 - DockerComposeV0: 0.263.0 -> 0.268.0 - DockerComposeV1: 1.265.0 -> 1.268.0 This ensures all Docker tasks in the repository handle BuildKit correctly and won't emit false warnings.
Improvements: - Added debug log when empty output is detected with BuildKit enabled This helps troubleshoot and confirms expected behavior - Consistent implementation across all Docker tasks - Better error messaging for users debugging their pipelines Unit Tests Added (DockerV2): - Test DOCKER_BUILDKIT=1 (explicit enable) - no warning expected - Test DOCKER_BUILDKIT=0 (explicit disable) - warning expected - Test DOCKER_BUILDKIT unset (modern default) - no warning expected - Test with actual output content - no warning regardless of setting These tests ensure the BuildKit detection logic works correctly across all three environment variable states and validates the fix for issue #17893.
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Reverted BuildKit detection approach as it made too many assumptions. Simpler solution: - Remove the 'No data was written' warning completely - Empty stdout is not a reliable error indicator (BuildKit, Docker Compose, etc.) - File is still created for debugging purposes - Actual errors are reported through Docker command exit codes This addresses issue #17893 without trying to detect Docker versions or BuildKit usage, which would be unreliable and incomplete. Note: The escapedCommandName in DockerCompose tasks was not changed by us - that's existing code for sanitizing command names.
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
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.
Context
Fixes #17893 - Docker@2 task incorrectly warns "No data was written into the file" when Docker BuildKit is in use.
Background: Since Docker 23.0 (February 2023), BuildKit became the default builder. BuildKit sends structured progress output to stderr instead of stdout, resulting in empty stdout which is expected behavior. The task was treating this as an error condition and warning users unnecessarily.
Task Name
DockerV2
DockerV1
DockerV0
DockerComposeV1
DockerComposeV0
Description
Remove warning as it doesn't make much sense with newer Docker versions. It is regular case to have empty stdout so this warning can be misleading that something was wrong.
Risk Assessment (Low / Medium / High)
Low.
Change Behind Feature Flag (Yes / No)
No. Warning logging is affected and this is bugfix, not a feature.
Tech Design / Approach
Documentation Changes Required (Yes/No)
No.
Unit Tests Added or Updated (Yes / No)
Yes.
Additional Testing Performed
No.
Logging Added/Updated (Yes/No)
Telemetry Added/Updated (Yes/No)
Rollback Scenario and Process (Yes/No)
Dependency Impact Assessed and Regression Tested (Yes/No)
Checklist