Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add PostgreSqlKernel #3688

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NikiforovAll
Copy link

resolves: #3056

@NikiforovAll
Copy link
Author

dup: #3676

@NikiforovAll
Copy link
Author

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

@jonsequitur
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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

@NikiforovAll
Copy link
Author

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

@NikiforovAll
Copy link
Author

TODO:

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

@jonsequitur
Copy link
Contributor

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.

@NikiforovAll
Copy link
Author

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

@jonsequitur
Copy link
Contributor

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.)

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.

add support for Postgres
2 participants