search

openzipkin / zipkin-api

Zipkin's language independent model and HTTP Api Definitions
https://zipkin.io/zipkin-api/
Apache License 2.0
59 stars 32 forks source link

decide if we want to validate thrift on pull request or not #69

Closed codefromthecrypt closed 5 years ago

codefromthecrypt commented 5 years ago

Unlike swagger and proto, there's no pure javascript tool to validate or use thrifts. You have to rely on a OS binary to first generate the code. This means our travis config either has to invoke two tools or we have a binary dependency in our validate.test.js only for thrift.

another option is to simply not bother with thrift checking as we don't add anything to it anymore.

cc @apache/zipkin-committers

codefromthecrypt commented 5 years ago

fwiw we've never validated thrift on pull request, and incidentally until today even swagger validation was busted!

Now, we validate swagger and proto on pull request.

jcchavezs commented 5 years ago

I would not add validation for now as we are not changing it. Even if change I would not try to validate or until the necessity of a big change.

tir. 30. apr. 2019, 05:39 skrev Adrian Cole notifications@github.com:

fwiw we've never validated thrift on pull request, and incidentally until today even swagger validation was busted!

Now, we validate swagger and proto on pull request.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-zipkin-api/issues/69#issuecomment-487812659, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXOYAWSNXUD6LANPM4C5NTPS65NTANCNFSM4HJIIVOQ .

jeqo commented 5 years ago

I'm with @jcchavezs, if we don't add anything to thrift anymore, we should not add validation.

codefromthecrypt commented 5 years ago

rule of three