interactive icon indicating copy to clipboard operation
interactive copied to clipboard

add PostgreSqlKernel

Open NikiforovAll opened this issue 1 year ago • 4 comments

resolves: #3056

NikiforovAll avatar Oct 01 '24 09:10 NikiforovAll

dup: #3676

NikiforovAll avatar Oct 01 '24 09:10 NikiforovAll

Unable to build and test because:

./bulid.sh
❯ ./build.sh
Building NPM in directory src/polyglot-notebooks
~/dev/interactive/src/polyglot-notebooks ~/dev/interactive
npm WARN deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.

added 343 packages, and audited 344 packages in 21s

48 packages are looking for funding
  run `npm fund` for details

5 vulnerabilities (2 moderate, 3 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues, run:
  npm audit fix --force

Run `npm audit` for details.

> @microsoft/[email protected] compile
> npm run lint && tsc -p  ./ && npm run compile-library && npm run compile-es-module


> @microsoft/[email protected] lint
> eslint src --ext ts


> @microsoft/[email protected] compile-library
> rollup -c rollup.library.config.js -i src/index.ts -o lib/polyglot-notebooks.js


src/index.ts → lib/polyglot-notebooks.js...
(!) Circular dependency
src/kernel.ts -> src/kernelInvocationContext.ts -> src/kernel.ts
created lib/polyglot-notebooks.js in 6.1s

> @microsoft/[email protected] compile-es-module
> rollup -c rollup.es.config.js -i ./src/webview/activation.ts -o ./dist/activation.js


./src/webview/activation.ts → ./dist/activation.js...
(!) Circular dependency
src/kernel.ts -> src/kernelInvocationContext.ts -> src/kernel.ts
created ./dist/activation.js in 6.4s
~/dev/interactive
Building NPM in directory src/polyglot-notebooks-browser
~/dev/interactive/src/polyglot-notebooks-browser ~/dev/interactive

added 266 packages, and audited 267 packages in 21s

35 packages are looking for funding
  run `npm fund` for details

5 high severity vulnerabilities

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> [email protected] compile
> npm run rollup


> [email protected] rollup
> npm run compile-ci -- -i src/index.ts -o dist/dotnet-interactive.js


> [email protected] compile-ci
> rollup -c rollup.config.js --bundleConfigAsCjs -i src/index.ts -o dist/dotnet-interactive.js


src/index.ts → dist/dotnet-interactive.js...
[!] (plugin rpt2) RollupError: src/index.ts:3:15 - error TS2307: Cannot find module './polyglot-notebooks/commandsAndEvents' or its corresponding type declarations.

3 export * from "./polyglot-notebooks/commandsAndEvents";
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/index.ts

    at error (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup\dist\shared\rollup.js:349:30)
    at Object.error (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup\dist\shared\rollup.js:1703:20)
    at RollupContext.error (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\src\context.ts:35:17)
    at C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\src\diagnostics.ts:70:17
    at Array.forEach (<anonymous>)
    at printDiagnostics (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\src\diagnostics.ts:42:14)
    at typecheckFile (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\src\index.ts:67:3)
    at Object.<anonymous> (C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\src\index.ts:260:5)
    at Generator.next (<anonymous>)
    at C:\Users\Oleksii_Nikiforov\dev\interactive\src\polyglot-notebooks-browser\node_modules\rollup-plugin-typescript2\node_modules\tslib\tslib.es6.js:76:71


NikiforovAll avatar Oct 01 '24 09:10 NikiforovAll

It looks like you need to run this script in an elevated prompt: https://github.com/dotnet/interactive/blob/main/src/ensure-symlinks.ps1.

jonsequitur avatar Oct 01 '24 15:10 jonsequitur

@NikiforovAll Could you add some tests? Like the MSSQL tests, they can be gated by a conditional FactAttribute-derived type (similar to MsSqlFactAttribute) since we don't currently run database integration tests in CI.

jonsequitur avatar Oct 02 '24 18:10 jonsequitur

@NikiforovAll Are you planning to add tests? Do you need help with this PR?

jonsequitur avatar Oct 22 '24 22:10 jonsequitur

@jonsequitur Yes, sorry for the late reply. I plan to do it in one or two days.

NikiforovAll avatar Oct 23 '24 14:10 NikiforovAll

TODO:

  1. Install something like https://github.com/lorint/AdventureWorks-for-Postgres and test the test
  2. Add more tests

NikiforovAll avatar Oct 24 '24 08:10 NikiforovAll

Install something like https://github.com/lorint/AdventureWorks-for-Postgres and test the test

We currently don't automate this kind of integration test in our CI which is why for example our MSSQL tests are only run manually. So no need to include this step in the PR, in case that was what you had in mind.

jonsequitur avatar Oct 24 '24 16:10 jonsequitur

I would like to set up a test database to run tests locally.

NikiforovAll avatar Oct 24 '24 16:10 NikiforovAll

I'm happy to try the same setup and verify. Please just include instructions on how to do that in the dev guide. (I should add some as well for the SQL tests, now that I mention it.)

jonsequitur avatar Oct 24 '24 17:10 jonsequitur

@jonsequitur I've added instructions and tests. It seems like everything works as expected

NikiforovAll avatar Oct 26 '24 11:10 NikiforovAll

Could you please assist with failing CI?

NikiforovAll avatar Oct 26 '24 19:10 NikiforovAll

You'll need to rebase and account for this API which was renamed.

Build FAILED.

D:\a\_work\1\s\src\Microsoft.DotNet.Interactive.PostgreSql\PostgreSqlKernel.cs(115,16): error CS1061: 'CompositeKernel' does not contain a definition for 'AddKernelConnector' and no accessible extension method 'AddKernelConnector' accepting a first argument of type 'CompositeKernel' could be found (are you missing a using directive or an assembly reference?) [D:\a\_work\1\s\src\Microsoft.DotNet.Interactive.PostgreSql\Microsoft.DotNet.Interactive.PostgreSql.csproj]
    0 Warning(s)
    1 Error(s)

jonsequitur avatar Oct 28 '24 20:10 jonsequitur

@jonsequitur rebased and fixed. Locally I managed to run the tests

NikiforovAll avatar Oct 29 '24 08:10 NikiforovAll

This is looking great @NikiforovAll! I have one minor rename suggestion and otherwise it looks ready to merge.

jonsequitur avatar Oct 29 '24 16:10 jonsequitur

@jonsequitur renamed

NikiforovAll avatar Oct 29 '24 16:10 NikiforovAll