Skip to content

Bump protobuf v1.2.0#3116

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
thaJeztah:bump_protobuf
Apr 4, 2019
Merged

Bump protobuf v1.2.0#3116
crosbymichael merged 2 commits intocontainerd:masterfrom
thaJeztah:bump_protobuf

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Mar 20, 2019

Opening as "WIP", to make CI run here (bumping / regenerating caused some issues when I did the same in the swarmkit repository)

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 20, 2019

Looks like the XXX_unrecognized cause issues here as well (see moby/swarmkit#2837 for the discussion on SwarmKit)

gometalinter --config .gometalinter.json ./...
73runtime\v2\runhcs\service.go:836:2:warning: too few values in struct literal (compile) (staticcheck)
74runtime\v2\runhcs\service.go:836:2:warning: too few values in struct literal (compile) (staticcheck)
75runtime\v2\runhcs\service.go:859:2:warning: too few values in struct literal (compile) (staticcheck)
76runtime\v2\runhcs\service.go:859:2:warning: too few values in struct literal (compile) (staticcheck)
77runtime\v2\runhcs\service.go:836:2:warning: unused struct field too few values in struct literal (structcheck)
78runtime\v2\runhcs\service.go:859:2:warning: unused struct field too few values in struct literal (structcheck)
79runtime\v2\runhcs\service.go:836:2:warning: too few values in struct literal (unconvert)
80runtime\v2\runhcs\service.go:859:2:warning: too few values in struct literal (unconvert)
81runtime\v2\runhcs\service.go:836:2:warning: unused variable or constant too few values in struct literal (varcheck)
82runtime\v2\runhcs\service.go:859:2:warning: unused variable or constant too few values in struct literal (varcheck)

@thaJeztah thaJeztah force-pushed the bump_protobuf branch 2 times, most recently from 3451cf4 to eb5a58d Compare March 20, 2019 18:08
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 20, 2019

🤔

Ran 66 of 73 Specs in 71.805 seconds
SUCCESS! -- 66 Passed | 0 Failed | 0 Pending | 7 Skipped

...

Done. Your build exited with 1.

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 20, 2019

@stevvooe if you have time; perhaps you could give this a look; looking for the changes in protobuf, I found discussions where you were on about preserving the "extra" fields; I wasn't sure if we wanted to keep them or discard them (downside of keeping them could perhaps be "more overhead" / memory?)

@thaJeztah thaJeztah mentioned this pull request Apr 3, 2019
Merged
@thaJeztah thaJeztah force-pushed the bump_protobuf branch 3 times, most recently from d2b9c63 to 767c720 Compare April 3, 2019 21:21
thaJeztah added 2 commits April 3, 2019 23:27
@thaJeztah
Update protobuf v1.2.0
830b029 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Regenerate protobufs
2d11f5e 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Apr 3, 2019

This may cause extra memory usage, but I don't think we necessarily want to turn off extras without a good reason. This change will cause a little extra memory bloat, so that may be reason enough.

Looking at the generated code, I don't see any causes for compatibility concerns. If you want to be sure, you can run the integration tests from an older checkout against a daemon built with these changes.

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Apr 3, 2019

LGTM

@thaJeztah thaJeztah changed the title [WIP] Bump protobuf v1.2.0 Bump protobuf v1.2.0 Apr 3, 2019
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Apr 3, 2019

Thanks for looking! I saw they also have a v1.3.1 now, but taking it one step at a time (I think BuildKit was on 1.2.0, so although not related, trying to keep my deps a tiny-bit in sync

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 3, 2019 •
edited
Loading

Codecov Report

Merging #3116 into master will decrease coverage by 4.66%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3116      +/-   ##
==========================================
- Coverage   45.14%   40.47%   -4.67%     
==========================================
  Files         111       74      -37     
  Lines       11971     9902    -2069     
==========================================
- Hits         5404     4008    -1396     
+ Misses       5733     5328     -405     
+ Partials      834      566     -268
Flag Coverage Δ
#linux ?
#windows 40.47% <ø> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/native/native.go 1.79% <0%> (-41.26%) ⬇️
archive/tar.go 16.99% <0%> (-26.8%) ⬇️
metadata/snapshot.go 21.53% <0%> (-24.28%) ⬇️
cio/io.go 1.52% <0%> (-21.38%) ⬇️
content/local/writer.go 56.86% <0%> (-0.99%) ⬇️
gc/scheduler/scheduler.go 66.34% <0%> (-0.97%) ⬇️
oci/spec_opts.go 30.33% <0%> (-0.25%) ⬇️
mount/temp_unix.go
sys/reaper_linux.go
services/server/server_linux.go
... and 34 more

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 e16368d...2d11f5e. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Apr 4, 2019

LGTM

@crosbymichael crosbymichael merged commit b7183a0 into containerd:master Apr 4, 2019
@thaJeztah thaJeztah deleted the bump_protobuf branch April 4, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @stevvooe @codecov-io @crosbymichael