Tweak NPM (some improvements)#29
Conversation
kravets-levko
requested review from
arikfr,
moderakh,
superdupershant and
yunbodeng-db
as code owners
| - name: Run unit tests | ||
| run: | | ||
| npm ci | ||
| npm run build |
There was a problem hiding this comment.
for running the tests, don't we need to build the package to ensure the files are generated?
There was a problem hiding this comment.
prepare script is executed when you:
- run
npm installornpm ciin order to prepare package itself (but it doesn't when you install package into another project vianpm install <package-name>) - before
npm packandnpm publish
So after npm ci it will be completely ready for use
There was a problem hiding this comment.
Also I checked CI - unit tests are still passing (as well as e2e are running but fail with connection error), which basically confirms that it works 🙂
There was a problem hiding this comment.
Thanks @kravets-levko for the explanation.
one other question for you, not directly related to this PR though, as this has been failing for a while.
why is the second CI failing?
There was a problem hiding this comment.
It needs credentials which are passed through repository secrets. But Github does not pass secrets to PRs from forks (there are security reasons). Therefore it doesn't pass on such external PRs, but will pass on PRs within the repo and on master. Currently we're trying to find some solution for this
moderakh
left a comment
moderakh
left a comment
There was a problem hiding this comment.
LGTM. thanks @kravets-levko
Rust source changes (native/sea/src/connection.rs + statement.rs) deferred to kernel repo PR #29 (kernel-napi-statement-validity) since napi/src now lives in databricks-sql-kernel/napi/. SeaOperationBackend.ts conflict resolved using integration commit 3da7aa7 (combining sea-results fetch pipeline with sea-operation lifecycle helpers).
2 participants
Slightly improve #28