deps: remove deprecated gogo proto #206
Conversation
|
Still need to update the bytes for the windows side of the tests but I need to do that on a windows machine. Is there a way we can improve the tests so it isn't reliant on the exact byte pattern for marshal? |
|
I attempted to run the tests on Windows to get the new hashes but I seem to have had almost every test fail due to permission issues there. The CI is reporting a success there but there seems to be some test failures there. |
jsternberg
force-pushed
the
gogoproto-remove
branch
3 times, most recently
from
2030300
to
747a7ee
Compare
|
Looks like the hashes didn't change when testing with the CI so going to undo that part of the change. This is ready for review now. |
jsternberg
force-pushed
the
gogoproto-remove
branch
from
747a7ee
to
ab6df96
Compare
docker-bake.hcl
Outdated
| @@ -39,6 +39,14 @@ target "lint" { | |||
| } | |||
| } | |||
|
|
|||
| target "generate" { | |||
There was a problem hiding this comment.
we should have another target validating generation similar to buildkit one:
- https://github.com/moby/buildkit/blob/e15601a00fbef2805db1ed87be7bb88628ae926b/hack/dockerfiles/generated-files.Dockerfile#L42-L56
- https://github.com/moby/buildkit/blob/e15601a00fbef2805db1ed87be7bb88628ae926b/docker-bake.hcl#L172-L177
and add it to the validate job matrix:
fsutil/.github/workflows/ci.yml
Lines 23 to 27 in 43b9329
There was a problem hiding this comment.
Added this. I also redid part of the generate script because running protoc is very frustrating. We want the import path to be the fully qualified path so that buildkit can import wire.proto using the fully qualified path, but protoc was just not happy with the paths being different between projects so I just decided "we're doing this in a dockerfile anyway so I'll just write the protoc command there rather than go generate."
jsternberg
force-pushed
the
gogoproto-remove
branch
2 times, most recently
from
15b2032
to
5822a71
Compare
docker-bake.hcl
Outdated
| @@ -39,6 +39,20 @@ target "lint" { | |||
| } | |||
| } | |||
|
|
|||
| target "validate-generate" { | |||
|
PTAL @kzys |
jsternberg
force-pushed
the
gogoproto-remove
branch
2 times, most recently
from
04798cd
to
87977d6
Compare
hack/update-generated-files
Outdated
| : ${CONTINUOUS_INTEGRATION=} | ||
|
|
||
| progressFlag="" | ||
| if [ "$CONTINUOUS_INTEGRATION" == "true" ]; then progressFlag="--progress=plain"; fi |
There was a problem hiding this comment.
This is not needed as there is no tty allocated on GHA and will therefore use plain progress. I think you can just remove this script
hack/validate-generated-files
Outdated
There was a problem hiding this comment.
Not sure why we need this script, could we not just have it inlined in the dockerfile like https://github.com/moby/buildkit/blob/23a2b6fab3f654e67e4407d5f9ef496ced12f029/hack/dockerfiles/generated-files.Dockerfile#L42-L56?
There was a problem hiding this comment.
I just copied what I saw the existing update and validate scripts were doing. We can always do it a different way.
There was a problem hiding this comment.
I've updated all of the hack scripts and the dockerfiles to use this inline technique rather than put too much logic in the hack scripts. The hack scripts now just invoke the appropriate bake target and the validate is now inlined in the dockerfile for gomod and generated-files.
jsternberg
force-pushed
the
gogoproto-remove
branch
2 times, most recently
from
4879a07
to
ca54b7c
Compare
go.mod
Outdated
| github.com/moby/patternmatcher v0.5.0 | ||
| github.com/opencontainers/go-digest v1.0.0 | ||
| github.com/pkg/errors v0.9.1 | ||
| github.com/planetscale/vtprotobuf v0.6.0 |
There was a problem hiding this comment.
do we want to separate vtproto as well here related to moby/buildkit#5342 (comment)?
jsternberg
force-pushed
the
gogoproto-remove
branch
from
ca54b7c
to
1fc76d7
Compare
|
Removed the vtproto part of this and I'll create a separate PR for vtproto. |
This removes the deprecated gogo proto in favor of the standard implementation. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
gogoproto-remove
branch
from
1fc76d7
to
a340068
Compare
This removes the deprecated gogo proto in favor of the standard implementation.