docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 176 forks source link

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

Closed ulyssessouza closed 4 years ago

ulyssessouza commented 4 years ago

- What I did Replaced os.File for an interface in containerd/console (https://github.com/ulyssessouza/console/commit/f652dc3e99a9f4aa760deb9b4b28edb7c4e5001a) so that can be used by docker/buildx (https://github.com/ulyssessouza/buildx/commit/5941345e21ebda43723a475380135fe3741d6b3c) 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 commented 4 years ago

/cc @tonistiigi @tiborvass

thaJeztah commented 4 years ago

@ulyssessouza is this the only change in the fork? https://github.com/ulyssessouza/buildx/commit/5941345e21ebda43723a475380135fe3741d6b3c

ulyssessouza commented 4 years ago

@thaJeztah In buildx, yes. But there is another fork. The containerd/console with https://github.com/ulyssessouza/console/commit/f652dc3e99a9f4aa760deb9b4b28edb7c4e5001a 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[bot] commented 4 years ago

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%) :arrow_down:

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 commented 4 years ago

Thanks for clarifying; yes we should avoid forks if possible

ndeloof commented 4 years ago

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 commented 4 years ago

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