Skip to content

Add grpc-httpjson-transcoding dependency#1066

Merged
mattklein123 merged 1 commit into
envoyproxy:masterfrom
lizan:grpc_json_transcoding_deps
Jun 8, 2017
Merged

Add grpc-httpjson-transcoding dependency#1066
mattklein123 merged 1 commit into
envoyproxy:masterfrom
lizan:grpc_json_transcoding_deps

Conversation

@lizan
Copy link
Copy Markdown
Member

@lizan lizan commented Jun 8, 2017

for #501

@htuch
Copy link
Copy Markdown
Member

htuch commented Jun 8, 2017

Looks good, but can you add a description in the commit message of what you're adding? You'll have to wait for the merge of #1063 for this and add the updated CI hash when the Docker image is rebuilt.

Also, I wonder if this technically becomes an additional dependency - we're not going to be consuming this as prebuilts, but building each time in CI and developer build, since this is Bazel native. @mattklein123 WDYT?

@mattklein123
Copy link
Copy Markdown
Member

Yeah, I was concerned about that as well (no prebuilt). Not sure if we should have a hard rule on this or decide on a case by case basis? How long will it take to build this extra code? I assume not that long?

@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jun 8, 2017

Having another build tool support or invoking bazel inside the deps rule doesn't sound right. The library itself is tiny (~5k lines of code), and it compiles in a few seconds. Unless you're changing its source or compiler options, bazel will cache it so it won't be time consuming for developer builds.

@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jun 8, 2017

@htuch What description are you looking for? The subject is already in commit message.

Yes I will wait #1063 to confirm if the library will build on the CI images, but since there is no real code yet, it doesn't breaks CI.

@htuch
Copy link
Copy Markdown
Member

htuch commented Jun 8, 2017

@lizan Explain what these libraries actually do in the commit message, not just why they are being imported.

Add dependency to the library from
https://github.com/grpc-ecosystem/grpc-httpjson-transcoding to provide
a RESTful style HTTP/JSON interface for gRPC Service.

for envoyproxy#501
@lizan lizan force-pushed the grpc_json_transcoding_deps branch from 8c3bd2c to f7a477f Compare June 8, 2017 22:09
@lizan
Copy link
Copy Markdown
Member Author

lizan commented Jun 8, 2017

@htuch done

@mattklein123 mattklein123 merged commit bd525d5 into envoyproxy:master Jun 8, 2017
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants