Aperture SDK for Javascript #817
Conversation
There was a problem hiding this comment.
A few general comments:
- Do not copy over the
flowcontrol.protofile. Check how we are generating the stubs here, and copying them over using the script here - You will have to expose additional endpoints from the example server. Refer to aperture-go's example for reference.
- Refer to aperture-go's Dockerfile to correctly expose the example service along with a healthcheck for the validator to run against it
DariaKunoichi
force-pushed
the
nodejs_sdk
branch
from
50f1795 to
7c805c0
Compare
DariaKunoichi
force-pushed
the
nodejs_sdk
branch
from
e97ee15 to
be39c70
Compare
DariaKunoichi
force-pushed
the
nodejs_sdk
branch
from
1db5d4d to
20b29c8
Compare
Codecov ReportBase: 37.54% // Head: 3.30% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #817 +/- ##
==========================================
- Coverage 37.54% 3.30% -34.24%
==========================================
Files 286 286
Lines 20852 20852
==========================================
- Hits 7828 689 -7139
- Misses 12541 20095 +7554
+ Partials 483 68 -415
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Reviewed 11 of 20 files at r1, 3 of 6 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DariaKunoichi)
.circleci/continue-workflows.yml line 641 at r3 (raw file):
remote-repo-fingerprint: "c5:d8:c4:22:7e:ff:b5:c3:c3:32:c2:b7:54:c7:99:1b" aperture-js:
Are we doing releases for that SDK? If so, you need to add it to .circleci/post-release.yaml
sdks/aperture-js/LICENSE.txt line 1 at r3 (raw file):
MIT License
Is this a correct license, or should that be Apache?
| this.span.setAttribute(CHECK_RESPONSE_LABEL, JSON.stringify(this.checkResponse)); | ||
| this.span.setAttribute(FLOW_END_TIMESTAMP_LABEL, Date.now()); | ||
|
|
||
| let attr = this.span.attributes; |
There was a problem hiding this comment.
Nit: is this line needed?
There was a problem hiding this comment.
leftover from debugging, thanks!
### Description of change - Move routes to example folder - Run example server on port 8080 - Fix typos ##### Checklist - [ ] Tested in playground or other setup - [ ] Documentation is changed or added - [ ] Tests and/or benchmarks are included - [ ] Breaking changes <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/fluxninja/aperture/853) <!-- Reviewable:end -->
DariaKunoichi
force-pushed
the
nodejs_sdk
branch
from
9e211ac to
0b972c2
Compare
There was a problem hiding this comment.
Reviewable status: 8 of 32 files reviewed, 3 unresolved discussions (waiting on @DariaKunoichi, @jmichalak-fluxninja, and @kklimonda-fn)
.circleci/continue-workflows.yml line 641 at r3 (raw file):
Previously, kklimonda-fn (Krzysztof Klimonda) wrote…
Are we doing releases for that SDK? If so, you need to add it to
.circleci/post-release.yaml
We should release this to npm - I would need help from frontend team so it won't be done in this PR.
sdks/aperture-js/LICENSE.txt line 1 at r3 (raw file):
Previously, kklimonda-fn (Krzysztof Klimonda) wrote…
Is this a correct license, or should that be Apache?
I copied this from aperture-go so probably yes?
There was a problem hiding this comment.
Reviewed 1 of 12 files at r4, 1 of 16 files at r7, all commit messages.
Reviewable status: 10 of 32 files reviewed, 1 unresolved discussion (waiting on @DariaKunoichi and @jmichalak-fluxninja)
Description of change
Checklist
This change is