Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Bump buildx and fix file finalizer breaking the stdout fix #779

Merged
merged 2 commits into from
Jan 7, 2020

Conversation

ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Dec 3, 2019

- What I did
Replaced os.File for an interface in containerd/console (ulyssessouza/console@f652dc3) so that can be used by docker/buildx (ulyssessouza/buildx@5941345) and docker/app

- How I did it
Fork and override of the repos in Gopkg.toml

- How to verify it
Check that the broken output fix still working by repeating a docker app build exhaustively.

- Description for the changelog
Use a file interface from containerd/console to pass to buildx's printer

- A picture of a cute animal (not mandatory)
mastigando

@thaJeztah
Copy link
Member

/cc @tonistiigi @tiborvass

@thaJeztah
Copy link
Member

@ulyssessouza is this the only change in the fork? ulyssessouza/buildx@5941345

@ulyssessouza ulyssessouza changed the title Use fork of buildx to fix file finalizer [On Hold] Use fork of buildx to fix file finalizer Dec 3, 2019
@ulyssessouza
Copy link
Contributor Author

@thaJeztah In buildx, yes. But there is another fork. The containerd/console with ulyssessouza/console@f652dc3
I'll propose a PR on each repo.
I also think this PR could be merged after the merges (if the case) in the upstream repos to avoid the override to my repo.

The intent of this PR is first to validate the approach.

@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #779 into master will decrease coverage by 0.09%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #779     +/-   ##
=========================================
- Coverage   70.13%   70.03%   -0.1%     
=========================================
  Files          67       67             
  Lines        3676     3684      +8     
=========================================
+ Hits         2578     2580      +2     
- Misses        753      759      +6     
  Partials      345      345
Impacted Files Coverage Δ
internal/commands/build/build.go 60.99% <64.7%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3064a5d...864442b. Read the comment docs.

@thaJeztah
Copy link
Member

Thanks for clarifying; yes we should avoid forks if possible

@ndeloof
Copy link
Contributor

ndeloof commented Dec 3, 2019

I like the approach of offering a "consumer" counterpart for a PR to upstream project, which helps better unterstand the needs and if fix is actually relevant.
Maybe we should agree on a label/PR prefix/comment for such scenario.
Typically PR description could explicitely claim "This demonstrate containerd#123" DO NOT MERGE

@ulyssessouza ulyssessouza force-pushed the refactor-build branch 2 times, most recently from 123fd39 to 278c050 Compare December 13, 2019 15:53
@ulyssessouza ulyssessouza changed the title [On Hold] Use fork of buildx to fix file finalizer Bump buildx and fix file finalizer breaking the stdout fix Dec 13, 2019
Copy link
Contributor

@aiordache aiordache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulyssessouza I wonder if it's ok that so many dependencies were updated? 🤔

@ulyssessouza
Copy link
Contributor Author

@silvin-lubecki After reviewing, that looks like it's a side-effect of the bumps on buildx and containerd.

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Signed-off-by: ulyssessouza <ulyssessouza@gmail.com>
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@silvin-lubecki silvin-lubecki merged commit 6d698be into docker:master Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants