feat: ICICLE msm integration

Description
closes: #490
Unit tests already test the correctness of the MSM results.
Benchmark can be run via cargo bench --bench msm --features "test-srs icicle"

[ ] Fixed cudatoolkit inside nix shell is incompatible with cuda driver problem (causing CudaErrorInsufficientDriver err)
[ ] Tweak around benchmark code and setup
[ ] Update Changelog

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

[x] Targeted PR against correct branch (main)
[x] Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
[x] Wrote unit tests
[x] Updated relevant documentation in the code
[ ] Added a relevant changelog entry to the Pending section in CHANGELOG.md
[ ] Re-reviewed Files changed in the GitHub PR explorer

Current benchmark doesn't look right, the difference is only 5x, we should expect at least >10x speedup. In initial exploration, we observed 200x differences!
MSM with arkworks/12    time:   [20.128 ms 20.157 ms 20.189 ms]
MSM with arkworks/13    time:   [35.529 ms 35.653 ms 35.778 ms]
MSM with arkworks/14    time:   [67.961 ms 68.246 ms 68.623 ms]
MSM with arkworks/15    time:   [119.05 ms 119.43 ms 119.77 ms]
MSM with arkworks/16    time:   [216.39 ms 216.61 ms 216.91 ms]
MSM with arkworks/17    time:   [435.28 ms 435.69 ms 436.10 ms]
MSM with arkworks/18    time:   [820.43 ms 821.17 ms 822.05 ms]
MSM with arkworks/19    time:   [1.5854 s 1.5876 s 1.5902 s]
MSM with arkworks/20    time:   [2.9161 s 2.9178 s 2.9199 s]
MSM with arkworks/21    time:   [5.9400 s 5.9642 s 5.9849 s]
MSM with arkworks/22    time:   [11.604 s 11.664 s 11.716 s]

MSM with ICICLE/12      time:   [15.503 ms 15.512 ms 15.523 ms]
MSM with ICICLE/13      time:   [17.806 ms 17.834 ms 17.862 ms]
MSM with ICICLE/14      time:   [22.151 ms 22.192 ms 22.222 ms]
MSM with ICICLE/15      time:   [30.990 ms 31.176 ms 31.352 ms]
MSM with ICICLE/16      time:   [51.316 ms 51.570 ms 51.919 ms]
MSM with ICICLE/17      time:   [89.319 ms 89.740 ms 90.149 ms]
MSM with ICICLE/18      time:   [160.70 ms 161.77 ms 163.22 ms]
MSM with ICICLE/19      time:   [318.50 ms 319.18 ms 319.94 ms]
MSM with ICICLE/20      time:   [635.62 ms 637.99 ms 640.69 ms]
MSM with ICICLE/21      time:   [1.2574 s 1.2616 s 1.2680 s]
MSM with ICICLE/22      time:   [2.5118 s 2.5236 s 2.5354 s]

I'm so annoyed that I can't get my nix shell to work. outside it, i can compile and run icicle code, but inside I can only successfully compile, but any invocation of cuda FFI code would failed.
$ nix develop .#cudaShell
$ cargo test gpu --release --features icicle

runtime error trace
---- pcs::univariate_kzg::tests::gpu_end_to_end_test stdout ----
thread 'pcs::univariate_kzg::tests::gpu_end_to_end_test' panicked at primitives/src/pcs/univariate_kzg/mod.rs:1026:14:
test failed for bn254: IcicleError("IcicleError { icicle_error_code: InternalCudaError, cuda_error: Some(cudaErrorInsufficientDriver), reason: Some(\"Runtime CUDA error.\") }")
stack backtrace:
   0:     0x555555b12366 - std::backtrace_rs::backtrace::libunwind::trace::hbee8a7973eeb6c93
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/../../backtrace/src/backtrace/libunwind.rs:104:5
   1:     0x555555b12366 - std::backtrace_rs::backtrace::trace_unsynchronized::hc8ac75eea3aa6899
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x555555b12366 - std::sys_common::backtrace::_print_fmt::hc7f3e3b5298b1083
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:68:5
   3:     0x555555b12366 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hbb235daedd7c6190
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x555555b3b720 - core::fmt::rt::Argument::fmt::h76c38a80d925a410
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/fmt/rt.rs:142:9
   5:     0x555555b3b720 - core::fmt::write::h3ed6aeaa977c8e45
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/fmt/mod.rs:1120:17
   6:     0x555555b0fbff - std::io::Write::write_fmt::h1299aa7741865f2b
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/io/mod.rs:1810:15
   7:     0x555555b12144 - std::sys_common::backtrace::_print::h5d645a07e0fcfdbb
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:47:5
   8:     0x555555b12144 - std::sys_common::backtrace::print::h85035a511aafe7a8
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:34:9
   9:     0x555555b13eb7 - std::panicking::default_hook::{{closure}}::hcce8cea212785a25
  10:     0x555555b13b9d - std::panicking::default_hook::hf5fcb0f213fe709a
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:289:9
  11:     0x555555a8d207 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h60265c2dfa87ee34
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2029:9
  12:     0x555555a8d207 - test::test_main::{{closure}}::h77865bd3127078c6
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:138:21
  13:     0x555555b144d6 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::hbc5ccf4eb663e1e5
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2029:9
  14:     0x555555b144d6 - std::panicking::rust_panic_with_hook::h095fccf1dc9379ee
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:783:13
  15:     0x555555b14222 - std::panicking::begin_panic_handler::{{closure}}::h032ba12139b353db
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:657:13
  16:     0x555555b12866 - std::sys_common::backtrace::__rust_end_short_backtrace::h9259bc2ff8fd0f76
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:171:18
  17:     0x555555b13f80 - rust_begin_unwind
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5
  18:     0x5555555ace85 - core::panicking::panic_fmt::h784f20a50eaab275
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14
  19:     0x5555555ad393 - core::result::unwrap_failed::h03d8a5018196e1cd
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1649:5
  20:     0x555555793ade - jf_primitives::pcs::univariate_kzg::tests::gpu_end_to_end_test::h4a2c05e21a017c3d
  21:     0x555555915b69 - core::ops::function::FnOnce::call_once::h34aa7a3b7c3c87aa
  22:     0x555555a92adf - core::ops::function::FnOnce::call_once::h8dc6907944022cf6
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/ops/function.rs:250:5
  23:     0x555555a92adf - test::__rust_begin_short_backtrace::haae1a87433f1efb3
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:627:18
  24:     0x555555a91861 - test::run_test_in_process::{{closure}}::h8c7decfa7c14e152
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:650:60
  25:     0x555555a91861 - <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h19e6ff056d9d21e9
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272:9
  26:     0x555555a91861 - std::panicking::try::do_call::h89c848fcaa37c035
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:552:40
  27:     0x555555a91861 - std::panicking::try::h57ab3dc74e2839b8
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:516:19
  28:     0x555555a91861 - std::panic::catch_unwind::hfb6a1b1abc120fb9
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panic.rs:142:14
  29:     0x555555a91861 - test::run_test_in_process::h5ae2f9875edd562d
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:650:27
  30:     0x555555a91861 - test::run_test::{{closure}}::h35d7300d8928a067
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:573:43
  31:     0x555555a58b96 - test::run_test::{{closure}}::h7525ced405d23d1b
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/test/src/lib.rs:601:41
  32:     0x555555a58b96 - std::sys_common::backtrace::__rust_begin_short_backtrace::h4e7db78ce05afad8
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys_common/backtrace.rs:155:18
  33:     0x555555a5dbf7 - std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}::hcfbcb64f1a1b3482
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/thread/mod.rs:529:17
  34:     0x555555a5dbf7 - <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h9d89c5c4108bd689
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272:9
  35:     0x555555a5dbf7 - std::panicking::try::do_call::h8a4869bc94ec50c9
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:552:40
  36:     0x555555a5dbf7 - std::panicking::try::h9a576f20ff81ac30
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:516:19
  37:     0x555555a5dbf7 - std::panic::catch_unwind::hbcb4e3f860ef9830
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panic.rs:142:14
  38:     0x555555a5dbf7 - std::thread::Builder::spawn_unchecked_::{{closure}}::h93c79a6be1505948
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/thread/mod.rs:528:30
  39:     0x555555a5dbf7 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h426d96740c81bdaf
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/ops/function.rs:250:5
  40:     0x555555b19095 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::h12de4fc57affb195
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2015:9
  41:     0x555555b19095 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::h3c619f45059d5cf1
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2015:9
  42:     0x555555b19095 - std::sys::unix::thread::Thread::new::thread_start::hbac657605e4b7389
                               at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/sys/unix/thread.rs:108:17
  43:     0x7ffff76a2333 - start_thread
  44:     0x7ffff7724efc - __clone3
  45:                0x0 - <unknown>

I first suspected that clang-sys that icicle's build.rs relies on didn't get the correct env var, but even with explicit setting it to the proper clang and llvm bin and lib, I still run into the above error.
I might need to dig deeper into what actually happens during the FFI calling phase, which object files are being used, and which dynamic library were used to built these executables etc. to pinpoint the source.
p.s. since icicle's build script explicitly use /usr/local/cuda/lib64 , I'm also setting my CUDA_PATH etc to a local installation instead of installing a cudatoolkit inside a nix shell
        baseShell = with pkgs;
          clang15Stdenv.mkDerivation {
            name = "clang15-nix-shell";
            buildInputs = [
              argbash
              openssl
              pkg-config
              git
              nixpkgs-fmt

              cargo-with-nightly
              stableToolchain
              nightlyToolchain
              cargo-sort
              clang-tools_15
              clangStdenv
              llvm_15
            ] ++ lib.optionals stdenv.isDarwin
              [ darwin.apple_sdk.frameworks.Security ];

            CARGO_TARGET_DIR = "target/nix_rustc";

            shellHook = ''
              export RUST_BACKTRACE=full
              export PATH="$PATH:$(pwd)/target/debug:$(pwd)/target/release"
              # Prevent cargo aliases from using programs in `~/.cargo` to avoid conflicts with local rustup installations.
              export CARGO_HOME=$HOME/.cargo-nix

              # Ensure `cargo fmt` uses `rustfmt` from nightly.
              export RUSTFMT="${nightlyToolchain}/bin/rustfmt"

              export C_INCLUDE_PATH="${llvmPackages_15.libclang.lib}/lib/clang/${llvmPackages_15.libclang.version}/include"
              export CC="${clang-tools_15.clang}/bin/clang"
              export CXX="${clang-tools_15.clang}/bin/clang++"
              export AR="${llvm_15}/bin/llvm-ar"
              export CFLAGS="-mcpu=generic"

              # ensure clang-sys got the correct version
              export LLVM_CONFIG_PATH="${llvmPackages_15.llvm.dev}/bin/llvm-config"
              export LIBCLANG_PATH=${llvmPackages_15.libclang.lib}/lib
              export CLANG_PATH=${clang-tools_15.clang}/bin/clang

              # by default choose u64_backend
              export RUSTFLAGS='--cfg curve25519_dalek_backend="u64"'
            ''
              # install pre-commit hooks
              + self.check.${system}.pre-commit-check.shellHook;
          };

//....
        devShells = {
          # enter with `nix develop .#cudaShell`
          cudaShell = baseShell.overrideAttrs (oldAttrs: {
            # for GPU/CUDA env (e.g. to run ICICLE code)
            name = "cuda-env-shell";
            buildInputs = oldAttrs.buildInputs ++ [ cmake util-linux gcc11 ];
            # CXX is overridden to use gcc11 as icicle-curves's build scripts need them, but gcc12 is not supported
            shellHook = oldAttrs.shellHook + ''
              export CUDA_PATH=/usr/local/cuda
              export PATH="${pkgs.gcc11}/bin:$CUDA_PATH/bin:$CUDA_PATH/nvvm/bin:$PATH"
              export LD_LIBRARY_PATH="$CUDA_PATH/lib64:$LIBCLANG_PATH"
            '';
          });
        };

🤦 need more time on this

The code is ready for review, interestingly I have some naive "warmup" function that makes the benchmark more accurate, as the warmup takes a constant of ~200ms, which you won't see now:
Start:   Committing to polynomial of degree 6238
··Start:   Type Conversion: ark->ICICLE: Group
··End:     Type Conversion: ark->ICICLE: Group .....................................230.554µs
··Start:   Load group elements: CPU->GPU
··End:     Load group elements: CPU->GPU ...........................................78.152µs
··Start:   Type Conversion: ark->ICICLE: Scalar
··End:     Type Conversion: ark->ICICLE: Scalar ....................................54.771µs
··Start:   Load scalars: CPU->GPU
··End:     Load scalars: CPU->GPU ..................................................21.100µs
··Start:   GPU-accelerated MSM
··End:     GPU-accelerated MSM .....................................................25.347ms
··Start:   Load MSM result GPU->CPU
··End:     Load MSM result GPU->CPU ................................................10.292ms
··Start:   Type Conversion: ICICLE->ark: Group
··End:     Type Conversion: ICICLE->ark: Group .....................................13.050µs
End:     Committing to polynomial of degree 6238  ..................................37.062ms

There are some remaining tasks:

[ ] Test passing for subslice (right now construction is fine, but memory dropping still panic; note without subslice things are fine)
[ ] add batch_commit() API which depends on the subslice (because I don't just want exact multiple number of bases of scalars, but I want multiple poly of any degrees <= supported_degree to be committed in batch)
[ ] update criterion benchmark code


CredentialService: Separate persistance and crypto

The credentialService has two main functionalities at the moment which are persistance and crypto. For a better unit-testing experience I propose to separate this functionalities to a crypto-service and a persistance-service, connected by a factory or something similar.
The other way could be that the persistance execution moves to the controller and the persistance query moves to en entity-Repository.
Create an entity (with properties for encrypted and plain-information) would be a nice improvement too.

Separating the service into 2 sounds good to me. Definitely more testable. As for moving business logic into the controller not so much. I would like to keep the controllers as lean as possible.

Proposal:
maintain the CredentialService with the following changes

add credentialEntity (https://github.com/MichaelThessel/pwx/issues/32)
as doctrine does the persistence of the Entity, there is no need to add an extra persistence class
add a cryptoService-Class which pipes credentialObject for encrypting and decrypting

Sounds good to me. Feel free to implement it.

I started separating a AES-Factory and a RSA-Service including first tests.
Here: https://github.com/MichaelThessel/pwx/commit/ed7d7049560ab866d94784d2210c44ee39d3f10f
Here: https://github.com/MichaelThessel/pwx/commit/d307c15006991e38a9db4b51475782c0f0f085cd
What should happen in the else cases here or here

I think you are doing the mistake of mixing the encryption service with the credential service. Ideally the encryption service would know nothing about the credentials. I think if we go for Doctrine ORM we don't need the credential service anymore. We would use the CredentialRepository to retrieve entities. The entity itself would have preUpdate and postLoad event handlers that handle the encryption. If we then add a CredentialFactory to create new Credential entities we should have used all the basic patterns :)  and implemented a nice level of abstraction. Hope that makes sense. Let me know if not.

Sounds nice. Some days ago I asked about doing persistence in the controller. What I have meant was exactly that. Using Doctrine ORM in the controller to persist and delete the credentials. Using the preUpdate and postLoad-events sounds good. So what is the best approach, injecting a dependency in the Entity or make it by use-statements and instantiate the class in the entity?
Nevertheless i finished my idea and have with  https://github.com/rabbl/pwx/commit/2bf66d4ee60f3aba1e7ee514843c410d54055dca a version with separation of services that does have the same behavior like the old one. So i think a first step is done having smaller controller and unittests.
It could be the basis of your thoughts.
Let me know what do you think.

I think injecting the encryption service into the entity might be best. I will have a look at the pull request in soon.


fix: prevent Commits tab from disappearing

This is an attempt to fix #23 (previously made another pull request with the wrong issue number for the branch name).
Now, when the user navigates using the browser's back button, clicks on the repository's name, or clicks within the options within the Pull requests section, the Commits button should no longer disappear and should be accessible without a refresh.
https://github.com/user-attachments/assets/0cdffe3b-6c57-415b-8f24-41c1e5c7c5e2

https://github.com/user-attachments/assets/656c4118-6c65-419b-b806-5931cfb24bf0
It correctly fixes the issue with moving to "issues" page and coming back. But still fails when trying to navigate back from other pages.

From discussions page, if I try to come back to le-git-graph page, the commits UI breaks. (Expands to full screen width)
From actions page, if I try to come back to le-git-graph page, It doesn't work.

A more general solution is required to handle navigation altogether.

Hi @NirmalScaria, thanks for checking out what I have right now!
In addition to what you listed, I also noticed that if I visit any file or folder within the Code page, I can't navigate back to the Commits page. I'll definitely need to think over this some more to figure out why we can't navigate back to the Commits page.
On a side note - thank you for making such a cool extension! I love using git log --all --graph locally, and find it really cool that this extension lets us see all of that from GitHub.


[Bug] Delayed 404 page breaks build

Following recommendations found in the docs, I setup a delayed 404, ostensibly to prevent flashing for dynamic routes. But if the 404 view returns null, build fails.
I'm very early in porting my app to RS, so I set this up preemptively. That said, I think there's something wrong with a null view breaking build. Null is renderable in a react tree, so something else is going on here that's important beyond the current issue.
Also, what's the current best practice for dynamic routes? It's been a while since I've used RS, so I'm trying to catch back up.
Thanks!
import React, {useState, useEffect} from 'react';

export default function NotFoundView() {
  const [isReady, setIsReady] = useState(false);

  useEffect(() => {
    setIsReady(true);
  }, []);

  return isReady ? (

Oh no! We couldn't find that page :(

  ) : null;
}

[=-] 1/2 50% Infinity/s 0.0s   Error: Failed exporting HTML for URL 404 (<monorepo>/apps/sales/views/NotFou  ndView): Minified React error #321; visit https://reactjs.org/docs/error-decoder.html?invari  ant=321 for the full message or use the non-minified dev environment for full errors and add  itional helpful warnings.

  - static-app.js:5199 Z
    <monorepo>/apps/sales/.tmp/artifacts/static-app.js:5199:25

  - static-app.js:5358 exports.useEffect
    <monorepo>/apps/sales/.tmp/artifacts/static-app.js:5358:10

  - static-app.js:7379 NotFoundView
    <monorepo>/apps/sales/.tmp/artifacts/static-app.js:7379:58

  - react-dom-server.node.development.js:3043 processChild
    [monorepo]/[react-dom]/cjs/react-dom-server.node.development.js:3043:14

  - react-dom-server.node.development.js:2960 resolve
    [monorepo]/[react-dom]/cjs/react-dom-server.node.development.js:2960:5

  - react-dom-server.node.development.js:3435 ReactDOMServerRenderer.render
    [monorepo]/[react-dom]/cjs/react-dom-server.node.development.js:3435:22

  - react-dom-server.node.development.js:3373 ReactDOMServerRenderer.read
    [monorepo]/[react-dom]/cjs/react-dom-server.node.development.js:3373:29

  - react-dom-server.node.development.js:3988 renderToString
    [monorepo]/[react-dom]/cjs/react-dom-server.node.development.js:3988:27

  - exportRoute.js:137 renderToStringAndExtract
    [monorepo]/[react-static]/src/static/exportRoute.js:137:21

  - exportRoute.js:194 _callee$
    [monorepo]/[react-static]/src/static/exportRoute.js:194:15

  - runtime.js:45 tryCatch
    [monorepo]/[regenerator-runtime]/runtime.js:45:40

  - runtime.js:274 Generator.invoke [as _invoke]
    [monorepo]/[regenerator-runtime]/runtime.js:274:22

  - runtime.js:97 Generator.prototype.<computed> [as next]
    [monorepo]/[regenerator-runtime]/runtime.js:97:21

  - asyncToGenerator.js:3 asyncGeneratorStep
    [monorepo]/[@babel]/runtime/helpers/asyncToGenerator.js:3:24

  - asyncToGenerator.js:25 _next
    [monorepo]/[@babel]/runtime/helpers/asyncToGenerator.js:25:9

  - task_queues.js:97 processTicksAndRejections
    internal/process/task_queues.js:97:5

  Error: <<BLANK>>

Environment
  System:
    OS: macOS 10.15.3
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Memory: 5.64 GB / 64.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 13.10.1 - ~/.nvm/versions/node/v13.10.1/bin/node
    Yarn: 1.21.1 - /usr/local/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v13.10.1/bin/npm
  Browsers:
    Brave Browser: <IP_ADDRESS>
    Chrome: 81.0.4044.122
    Chrome Canary: 80.0.3980.0
    Firefox: 70.0.1
    Safari: 13.0.5

Steps to Reproduce the problem

Just use the 404 view shown above.

Expected Behavior
I expect to be able to export views that return null.

Try to return an empty div instead? My guess is that it's checking for a truthy value.

I did, and it does work. I just don't think it should be the default behavior, given how react typically behaves. On the other hand, I can't seem to track down where the error is being thrown. It doesn't look like it's on the command/export side or the components/Routes side.

I agree that the behaviour is suboptimal -- you probably want to look where it's determining which route is 404, and that it exists, and that it renders something. I don't know by heart where that's happening tho ⭕


Clean-up database usage in unittests, major upgrade do not merge

Major upgrade: remove reallyDeleteMyDatabaseAfterEveryTest bot option and replace it with setUp/tearDown approach to setup/delete unittest databases for every unit test; re-factor DBCore class to use generators and be compatible with WMCore functionality; re-factor ResultSet to keep data instead of resultproxy objects; close all resultproxy objects once data is read from them; change logic of processData to use transaction (it as reversed); adjust all unit tests to create its own database for their test purposes

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/589/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/590/ for details

FWIW, the 'really tear down database'  option is there because otherwise it
literally will blow away the entire database. I didn't want someone to
accidentally run the command and have it wipe whatever databases were
around.

Is there no way we can do this more gradually? Also, why are you changing the DBS3Reader code?

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/591/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/592/ for details

Jenkins is going to flail on this:
error: option --reallyDeleteMyDatabaseAfterEveryTest not recognized
If you want to see a test, commit something that allows this option but doesn't do anything with it.

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/593/ for details

Good point, I put it back as an option.
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

Jenkins is going to flail on this:
error: option --reallyDeleteMyDatabaseAfterEveryTest not recognized
If you want to see a test, commit something that allows this option but doesn't do anything with it.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141555557

The short answer is no, I changed internals of WMCore which affect
almost entire universe. But changes are isolated to WMCore and affect unit
tests.
The changes for DBS3 are minor to comply with overall
change, i.e. specify transactions, use prepareDatabase method in unit tests
and changes for column names in tables which were restricted by maria db.
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

Is there no way we can do this more gradually? Also, why are you changing the DBS3Reader code?

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141545075

Please don't post comments without context. First, read the changes.  The hack option
"reallyDeleteMyDatabaseAfterEveryTest" is replaced with correct setUp/tearDown
procedure. Every unit test creates its own DB for itself and there is not need
anymore for reallyDeleteMyDatabaseAfterEveryTest option and, this option didn't
used in production either.
On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

FWIW, the 'really tear down database'  option is there because otherwise it
literally will blow away the entire database. I didn't want someone to
accidentally run the command and have it wipe whatever databases were
around.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141544846

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/594/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/595/ for details

Hi @vkuznet - I didn't just post off the hip, the option was there for a reason and I checked to look at the code. First the history of why it was there:
If the setUp/tearDown works right, then you always get your database back to a clean state. If not, the database is left inconsistent, and next time you run the test, the setUp blows up.
If you're a developer working on your own machine, you can sort it out and do the right thing with the database, but what ended up happening the first time around with jenkins is a test case would be broken, not tear down properly and then break every single test case after. Worse, it would also break every single test that came after.
The option to delete the database doesn't simply call the Delete DAOs after each test like you've done here:
https://github.com/vkuznet/WMCore/blob/a1c508fbb43a089e86c1d01e4d1129a80fe106d7/src/python/WMCore/WMInit.py#L211
via
https://github.com/vkuznet/WMCore/blob/a1c508fbb43a089e86c1d01e4d1129a80fe106d7/src/python/WMQuality/TestInit.py#L269
It actually does a DROP DATABASE; CREATE DATABASE to ensure the database is truly empty. Otherwise Eric or whomever will have to periodically clean out the mysql databases to deal with Jenkins blowing up.

Sorry, I needed to go one level deeper. You do blow away the whole database, but now it's the default, so if someone who happens to have their WMAGENT_CONFIG set to production stuff runs test code, it deletes everything for them without warning...

"If the setUp/tearDown works right, then you always get your database back to a clean state. If not, the database is left inconsistent, and next time you run the test, the setUp blows up."
Python 2.7 includes (as @alexanderrichards reminded us) an .addCleanup method in setup that will run even if tearDown does not, so that can be used to solve that particular problem later this year.

On Sep 18, 2015, at 15:50, Andrew Melo<EMAIL_ADDRESS>wrote:
Sorry, I needed to go one level deeper. You do blow away the whole database, but now it's the default, so if someone who happens to have their WMAGENT_CONFIG set to production stuff runs test code, it deletes everything for them without warning…

I presumed this is what you were worried about in the first place. I’m afraid I don’t really know what the solution is to have guaranteed clean unit tests but also not blow away everything is some well meaning person runs a unit test on a production server.

@ericvaandering It's not just a matter of if the tearDown doesn't fire, which is obnoxious. People also had an annoying habit of changing their Create DAOs and not their Delete DAOs.
In either case, I think that having the default behavior to be to completely drop and recreate the database is a BAD idea.

@ericvaandering The solution is to require people to opt in with a very clear command line argument.
Newer versions of binutils won't let you rm -rf /, you have to pass an additional argument to say "yes, I'm really sure what I want to do is recursively delete everything. You don't lose the functionality, but it keeps most of the trivial accidents from ruining your day.

I would like to get away from having to run tests with setup_test.py (or at least having to run them that way to get the same results that Jenkins gets). Requiring export REALLY_DROP_DATABASES=I_AGREE in the environment may be safe enough.

Or even writing a plugin to add a command line option to nose itself: http://nose.readthedocs.org/en/latest/plugins/writing.html

Andrew,

If the setUp/tearDown works right, then you always get your database back to a clean state. If not, the database is left inconsistent, and next time you run the test, the setUp blows up.

the prepareDataset which is invoked in setUp does five steps. If dataset left
in unclean case it will be dropped first, then new dataset will be created,
schema will be populated and connection will be reset to fresh new dataset for
test to run. All this steps are done at every setUp.
Moreover, every tests runs with its own databases, i.e. one unit tests can't run with
database from another test. The current design is opposite. All unit tests
are run within the same dataset and if something left over there we have
failures. This is why the current design is not unittests per-se it is
integration tests.

If you're a developer working on your own machine, you can sort it out and do the right thing with the database, but what ended up happening the first time around with jenkins is a test case would be broken, not tear down properly and then break every single test case after. Worse, it would also break every single test that came after.

The purpose of unit tests is to have clean DB populate with its own stuff and
run tests with it. If test fails and tearDown is not invoked, e.g. due to
exception you have:

a unique dataset to look for error
no other tests contribute to this database which is nice to isolate the
problem

The option to delete the database doesn't simply call the Delete DAOs after each test like you've done here:
https://github.com/vkuznet/WMCore/blob/a1c508fbb43a089e86c1d01e4d1129a80fe106d7/src/python/WMCore/WMInit.py#L211
via
https://github.com/vkuznet/WMCore/blob/a1c508fbb43a089e86c1d01e4d1129a80fe106d7/src/python/WMQuality/TestInit.py#L269
It actually does a DROP DATABASE; CREATE DATABASE to ensure the database is truly empty. Otherwise Eric or whomever will have to periodically clean out the mysql databases to deal with Jenkins blowing up.

I made only separation of methods and nothing else.
The current behavior is in this class
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Database/MySQL/Destroy.py
which does all steps at once, drop database, create database and use it.
What I did is separate the logic into individual classes. One class destroy
database and another create it.

I want to understand the logic here.
I repeat again, the commit made ALL unittests to create their own unique
databases. How they can affect your WMAGENT_CONFIG database?
For example DBCore_t.py test creates unitttest_DBCore, the JobAccounting_t.py
creates unittest_JobAccounting database, etc. Please explain how it affects
your database which you define in WMAGENT_CONFIG?
Again, if you need integration tests, then you can and will use WMAGENT_CONFIG
to point to YOUR database and we can adjust code to write over there.
On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

Sorry, I needed to go one level deeper. You do blow away the whole database, but now it's the default, so if someone who happens to have their WMAGENT_CONFIG set to production stuff runs test code, it deletes everything for them without warning...

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141565444

Why person is allowed to run anything on production server?
The unit tests are not supposed to be run on production server. For that people
must use pre-production server.
Again, all unit tests creates their own DB and CAN NOT affect any production
database by definition now. Isn't it a good thing?
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

On Sep 18, 2015, at 15:50, Andrew Melo<EMAIL_ADDRESS>wrote:
Sorry, I needed to go one level deeper. You do blow away the whole database, but now it's the default, so if someone who happens to have their WMAGENT_CONFIG set to production stuff runs test code, it deletes everything for them without warning…

I presumed this is what you were worried about in the first place. I’m afraid I don’t really know what the solution is to have guaranteed clean unit tests but also not blow away everything is some well meaning person runs a unit test on a production server.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141568076

On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

@ericvaandering It's not just a matter of if the tearDown doesn't fire, which is obnoxious. People also had an annoying habit of changing their Create DAOs and not their Delete DAOs.

why do you accept code which modifies Create DAOs and does not do proper stuff
to Delete DAOs. I don't see ANY REASON why code should be accepted in a first
place. Do not try to hide a real problem.

In either case, I think that having the default behavior to be to completely drop and recreate the database is a BAD idea.

I'll not repeat myself here, please read previous comments.

@vkuznet I guess there's just a difference of opinion.
I think there should be a safety on the trigger that blows away the whole database. You think that nobody would ever accidentally run the test harness on an important system, so the safety is unnecessary.

Eric and Alexander are correct here. Right now I modified setUp to take care
of left overs by dropping DB and re-creating it. Next logical step will
be to use .addCleanup method. But in order to use it the changes I made
to separate databases across all unit tests are required.
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

"If the setUp/tearDown works right, then you always get your database back to a clean state. If not, the database is left inconsistent, and next time you run the test, the setUp blows up."
Python 2.7 includes (as @alexanderrichards reminded us) an .addCleanup method in setup that will run even if tearDown does not, so that can be used to solve that particular problem later this year.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141567770

Yes, I stand on position that unittests should not be run on production nodes.
For that purpose we must use pre-production setup.  The code should run in
sandbox environment where nobody can fish.   We don't need special triggers
if code is correctly designed. Triggers leads to hacks, abandoning
the system and end-less if-else-if-else-if statements in a code (which is what
I see over there).
On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

@vkuznet I guess there's just a difference of opinion.
I think there should be a safety on the trigger that blows away the whole database. You think that nobody would ever accidentally run the test harness on an important system, so the safety is unnecessary.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141579149

Like you said, as long as nobody screws up and runs the code in the wrong place, everything is gravy :)

I said that code now designed to setup and run stuff in sandbox environments.
If you accessing production system and have all rights over there nothing
can stop you to make any mistake and screw the system, triggers is not
a remedy for admin rights.
On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

Like you said, as long as nobody screws up and runs the code in the wrong place, everything is gravy :)

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141581453

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/599/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/598/ for details

Can you guys stop going back and forth until I can find time to weigh in?
Sent from a mobile device. Please excuse my brevity or transcription errors.
On Sep 18, 2015, at 17:18, Valentin Kuznetsov <notifications<EMAIL_ADDRESS>wrote:
I said that code now designed to setup and run stuff in sandbox environments.
If you accessing production system and have all rights over there nothing
can stop you to make any mistake and screw the system, triggers is not
a remedy for admin rights.
On 0, Andrew Melo <notifications<EMAIL_ADDRESS>wrote:

Like you said, as long as nobody screws up and runs the code in the wrong place, everything is gravy :)

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141581453

—
Reply to this email directly or view it on GitHubhttps://github.com/dmwm/WMCore/pull/6260#issuecomment-141582136.

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/601/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/602/ for details

Ok, I am now free from shuttling our speaker back and forth. I'm going to lay out my understanding and then you can both (all?) tell me where I am wrong.
First, I did not appreciate that Valentin's patch uses a differently named database for every single test. Given that, I cannot see how blowing away that specific database could cause an issue as long as we have a guarantee that the testing code never ever creates, writes to, or drops a database with a different, universal name.
Second, I think we all agree that no one should run tests on "production" servers. However, it has been standard practice to run tests on developer's personal servers. While inadvertently blowing away a database in that case will not cause dataloss to CMS, it could certainly ruin what a developer is trying to do. Furthermore, just because something should not be done does not mean it will not happen. WMAgent doesn't just have knowledgeable core developers, there are casual developers and ops people as well. So a "belt and suspenders" approach is warranted.
The same is true of code review type questions. Yes, we try to catch mistakes. No one can guarantee they are all caught.
Finally, it seems that python has given us better tools (certainly it will with 2.7) to manage our testing framework. We should definitely be looking at how we can use them.

At the risk of opening another can of worms, how do we solve similar issues for Couch and not just MySQL/MariaDB?

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/603/ for details

The tests are failing because the Jenkins jobs are taking much longer (a few times more than usual) for this PR. I've bumped up the timeouts to 4x longer than the average successful job. Hopefully that will help.

@ericvaandering I wasn't trying to start a whole thing -- I was just trying explain why it was there. Like you said, it's a belt and suspenders thing.
On one hand, it's good to mitigate the chances of fumbling a command and blowing away something important. On the other hand, if you're testing locally on your machine, it should fail loudly if your code doesn't tear down the database properly and not paper over that failure (IMO). If the local code doesn't Do The Right Thing and tear things down right, the tests won't fail and whoever reviews the code will accept it, meaning that faulty behavior lives on.
That's the point I was (unsuccessfully) trying to articulate.

As an aside, I'm kinda curious about On 0, Andrew Melo <notifications<EMAIL_ADDRESS>wrote:. Did I time-travel to jan 1, 1970 (or whenever the unix epoch is)?

Not having had a clue about how this really worked before today.... Your point is (rephrasing) that if the DeleteDAO and CreateDAO don't match, we won't know if we are dropping the DB?

Maybe today doesn't exist yet in the Julian calendar. :-)

Unless I'm misreading the code, the database is blown away unconditionally during test setUp here: https://github.com/vkuznet/WMCore/blob/a1c508fbb43a089e86c1d01e4d1129a80fe106d7/src/python/WMQuality/TestInit.py#L269
..without letting the user/tester know if there were scraps sitting around in the database from previous tests.

"without letting the user/tester know if there were scraps sitting around in the database from previous tests."
What happens in unittest if we were to check if the DB is empty in the tearDown and throw an exception if it wasn't? If that works, it might actually do a better job of localizing these errors. As it is, we only get problems if we run a test after the one that failed and then the reasons are obscured.

You're right, that would be ideal. One way or another, waving a flag and saying, "this is not good" helps keep the riff-raff out :)

My question can be simplified to "what happens if we throw an exception in tearDown()". Is that an OK thing to do?

I'm going to claim ignorance in how it behaves in 2.7+. In 2.6 at least, you have the problem where if you want to do something like:
def tearDown(self):
    cleanA()
    cleanB()

and cleanA() throws, cleanB() won't fire. Depending on what cleanB() does, it might or might not matter, but you could see a couchDB or a temporary directory being torn down in that case.

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/604/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/605/ for details

Beginning pylint and unit tests.
See https://cmssdt.cern.ch/jenkins/job/DMWM-PR-test/606/ for details

@vkuznet Even after increasing the timeout, many of the slices of this patch cannot complete. The test suite runs some 5-6 times slower I'm guessing. Also, the slices that do complete show a large increase in the number of errors (triple or so as you can see here: https://cmssdt.cern.ch/jenkins/view/DMWM/job/DMWM-PR-unittests/SLICE=2,label=slc6_amd64-cloud/523/testReport/)

Let me make an alternate proposal and you can tell me if it makes sense:
First, this PR also appears to include the generator from databases stuff that Valentin has been telling us about the last couple of weeks. But isn't that orthogonal to testing harness issues?
Can we leave the existing mechanism in place with it's repeated dropping and creating of a shared database and instead start with a new RESTBaseUnitTestWithDBBackend.py (add a 2 if need be) that does the setup/teardown the way Valentin suggests, uses unique database names, maybe gets us ready for the nice thing(s) in 2.7, etc. Then we can do a staged migration to this?
From the test results its also clear that this modified class is not picking up all the database usage (I doubt it was intended to) and we may need a "UnitTestWithDB" class as well for things like the WMBS tests.
I assume the "reallyDropDatabases" flag on the legacy database and the new behavior on uniquely named databases can co-exist.

Hi,
to clarify changes I made I want to point out to the existing WMCore code behavior.  For that I'll use existing DBCore_t.py file [1] in master head of WMCore as an example. All other WMCore tests do EXACTLY the same, therefore logic is applied to every database related unittest.
Code [1] does few things,
For a DATABASE environment variable user set before hand of the tests the
[1] does few things:

in setUp it calls self.testInit.setDatabaseConnection() method

this cause a WMinit call to setDatabaseConnection, see [3], which creates
MySQL object via DBFactory

in tearDown it calls self.testInit.clearDatabase() which will call WMinit
clearDatabase() method which by itself calls Destroy class of MySQL object [2]
The Destroy class does 3 things DROP the database, CREATE the database and
USE the database, see [2].

the setup_test.py uses reallyDeleteMyDatabaseAfterEveryTest flag to flip
global variable in [5] which if it sets will blow up database via [2] anyway.
Moreover it is done in setDatabaseConnection of [5].

To sum-up, the current design does blow up database if reallyDeleteMyDatabaseAfterEveryTest is set. If it is not the case the database content is carried over from one test to another.
What I did is separated [2] logic into Destroy and Create classes whose names clearly identify their action over DB. Remove global variables and reallyDeleteMyDatabaseAfterEveryTest and set every unittest to use its own DB. I also align logic of transactions in a code
If WMCore team would like to preserve current behavior then the following
changes can be made on top of my patch:

set testDB to unique database name, i.e. read it from environment variable
destroy DB content conditionally, i.e. read a glad from environment variable
It can be easily accommodated in a clean way.

Best,
Valentin.
[1] https://github.com/dmwm/WMCore/blob/master/test/python/WMCore_t/Database_t/DBCore_t.py
[2] https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Database/MySQL/Destroy.py
[3] https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMInit.py
[4] https://github.com/dmwm/WMCore/blob/master/setup_test.py
[5] https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/TestInit.py

Andrew,
Of course you're right on this subject and I didn't changed anything in current
code behavior. Current tests are also have tearDown issue you pointed out, I
neither invent or had any intention to change it.
Here is example from current WMCore (all other tests do the same):
https://github.com/dmwm/WMCore/blob/master/test/python/WMComponent_t/JobAccountant_t/JobAccountant_t.py#L96
and  self.testInit.clearDatabase() will call MySQL Destroy class
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Database/MySQL/Destroy.py
which drops and create database again.
If the exception is thrown in self.testInit.clearDataset(), yes the logic will be the same in
current code too, i.e. the self.testInit.tearDownCouch() will not be called.
On  0, Andrew Melo<EMAIL_ADDRESS>wrote:

I'm going to claim ignorance in how it behaves in 2.7+. In 2.6 at least, you have the problem where if you want to do something like:
def tearDown(self):
    cleanA()
    cleanB()

and cleanA() throws, cleanB() won't fire. Depending on what cleanB() does, it might or might not matter, but you could see a couchDB or a temporary directory being torn down in that case.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141619594

Eric,
I'm not sure what you meant by that, but currently couch setup is more correct
design wise. We connect to Couch, then create db there and tear it down.
With my PR I shown that it can be down in similar way for MySQL/MariaDB.
V.
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

At the risk of opening another can of worms, how do we solve similar issues for Couch and not just MySQL/MariaDB?

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141617355

Eric, I can explain time in my design proposal, every unit tests drop database,
create database and create all tables. In current system I don't really know
what's going on, since it depends how BuildBot is run (does it uses
reallyDeleteMyDatabaseAfterEveryTest flag or not), does global variables
changes throw the tests (I didn't inspect all tests classes and have no idea if
someone didn't overwrite globals), etc.  The increase of failures are related
to two facts. I trip down database column names (Alan's comment) due to
constrain from MariaDB and I don't know if shared data is left among tests. My
point is that in current system I can't even say for sure what's going on.
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

@vkuznet Even after increasing the timeout, many of the slices of this patch cannot complete. The test suite runs some 5-6 times slower I'm guessing. Also, the slices that do complete show a large increase in the number of errors (triple or so as you can see here: https://cmssdt.cern.ch/jenkins/view/DMWM/job/DMWM-PR-unittests/SLICE=2,label=slc6_amd64-cloud/523/testReport/)

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141661988

First, this PR also appears to include the generator from databases stuff that Valentin has been telling us about the last couple of weeks. But isn't that orthogonal to testing harness issues?

this can go into separate PR indeed.

Can we leave the existing mechanism in place with it's repeated dropping and creating of a shared database and instead start with a new RESTBaseUnitTestWithDBBackend.py (add a 2 if need be) that does the setup/teardown the way Valentin suggests, uses unique database names, maybe gets us ready for the nice thing(s) in 2.7, etc. Then we can do a staged migration to this?

I am not sure I'm following. Please review the current WMCore test behavior I
posted and really say what logic of TestInit setUp and tearDown should do.
Since I managed to separate Dropping DB and its creation, adjust method to do
what initial logic was meant to do, now we can put things together as plan will
clearly define. I am not sure I understand the purpose of new
RESTBaseUnitTestWithDBBackend.py and how it fits in unittest picture (I think
we're talking about all database releated unittests here).

From the test results its also clear that this modified class is not picking up all the database usage (I doubt it was intended to) and we may need a "UnitTestWithDB" class as well for things like the WMBS tests.

Here I agree, if WMBS wants to perform integration tests we need new class
which they should use for that. The new class will setup once a database
and let re-use it within integration tests. The setup of database will be done in init/del
methods rather than in setUp/tearDown.

I assume the "reallyDropDatabases" flag on the legacy database and the new behavior on uniquely named databases can co-exist.

This can be accomplished in new design proposal via usage of environment
variables rather than globals and hacks in setup_tests.

Let me be more clear on my proposal. I am not saying to leave this as it is forever. But I'm leery of big patches that change everything. This is especially difficult since we have another "big patch" outstanding and the two are bound to collide.
So what I would rather see, if possible, is a small patch that adds a new class that "does it right" and just a few tests that use it. Then we can work on migrating the rest until they are all done. That new class should really use inheritance to move as much of the "action" as possible into the new class.
I realize this probably makes TestInit (I think that was the code) a bit more complex in the mean time since it has to have methods to support the new way of doing things and the old, but IMO, this is a more deliberate, conservative way to approach this.
I realize what we have been doing may not be optimal, but it has worked for us, more or less, for several years. Taking 6 months to transition over is not going to kill us.
I'll come back to some of the more minor points next week.

Hi all, Sorry for joining the conversation late.
Here are some of my opinion discussed here.

This patch should only concentrate on the unittest setting. (For generator and fix for transaction and connection handling should be separate patches - each)

I think it is good idea creating database and destroying database is good idea for each unittest. It is a cleaner solution than keep tracking the tables and drop the necessary tables. I understand the concern about destroying unwanted database but we can add more safety measure.
a. for the production deployment doesn't contain unittest code (currently implemented).
b. we absolutely avoid creating the same database name as production database wmagent, reqmgr, etc. (We can have a specific rule for unittest db as Valentin showed - unittest prefix and unittest will auto generate the db name when we set up the test. Also for tearDown, we can always do the Lexicon matching to check whether db contains the prefix before delete them)

I think initial patch should just include the testInit changes. Basically make this function to do following.
https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/TestInit.py#L158
a.  if there is db exist for particular unittest delete. ( this can be skipped if we use python 2.7 addcleanup)
b. create the database
c. create the db connection

And tearDown method will drop the db.
That way I don't think individual unittest code need to be changed initially and avoid conflict with other unittest fix. After unittest fix are upto the certain standard we can change to proper method name or divide to multiple methods.
Misc:
I think we can move  the unittest rpm setting to python 2.7 now (and use addcleanup - but not just for that), since we have the repository. (There is concern that we test the code against python 2.7 and production runs 2.6 but we need to do this eventually anyway and current code is python 2.7 compatible. - One concern is Tier0 code which shares the same unittest strtucture. But I think it is not much to change Tier0 to make python 2.7 ready)

One more thing: As others said, we can't include the schema changes here. (reducing the variable length).  I get the same error Valentin gets. (I set up unittest env manually not following the wiki in local not following.) :smile:  But we use the same set up in production (MariaDB with the same schema). I wonder why we don't have the same problem. (We had but I thought Alan fixed)
https://github.com/dmwm/WMCore/pull/5588

Seangchan,
if I read changes correctly, Alan's patch does not really address issue with unique indexes.
It is something else on DB setup which needs to be done (may be at compile time). If it is
configuration parameter before compiling the database it explains difference in VM
deployment versus local setup we had.
Valentin.
On  0, ticoann<EMAIL_ADDRESS>wrote:

One more thing: As others said, we can't include the schema changes here. (reducing the variable length).  I get the same error Valentin gets. (I set up unittest env manually not following the wiki in local not following.) :smile:  But we use the same set up in production (MariaDB with the same schema). I wonder why we don't have the same problem. (We had but I thought Alan fixed)
https://github.com/dmwm/WMCore/pull/5588

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-142042431

This patch should only concentrate on the unittest setting. (For generator and fix for transaction and connection handling should be separate patches - each)

agreed, I'll provide separate patches.

I think it is good idea creating database and destroying database is good idea for each unittest. It is a cleaner solution than keep tracking the tables and drop the necessary tables. I understand the concern about destroying unwanted database but we can add more safety measure.
a. for the production deployment doesn't contain unittest code (currently implemented).
b. we absolutely avoid creating the same database name as production database wmagent, reqmgr, etc. (We can have a specific rule for unittest db as Valentin showed - unittest prefix and unittest will auto generate the db name when we set up the test. Also for tearDown, we can always do the Lexicon matching to check whether db contains the prefix before delete them)

I think initial patch should just include the testInit changes. Basically make this function to do following.
https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/TestInit.py#L158
a.  if there is db exist for particular unittest delete. ( this can be skipped if we use python 2.7 addcleanup)
b. create the database
c. create the db connection

And tearDown method will drop the db.
That way I don't think individual unittest code need to be changed initially and avoid conflict with other unittest fix. After unittest fix are upto the certain standard we can change to proper method name or divide to multiple methods.

For this plan to work we need to re-factor TestInit internals several times. In
current implementation there are two methods:

all unit tests call setDatabaseConnection() in their setUp() methods
therefore we need to implement your a/b/c steps over there
all unit tests call clearDatabase() in their tearDown() methods
therefore we need to implement deletion of database over there

I would assume that we'll create proper internal methods in TestInit which can
be used in re-factoring I mentioned above. Then we'll need to re-factor
TestInit again to replace setDatabaseConnection() and clearDatabase() with
proper structure. Am I correct?

Misc:
I think we can move  the unittest rpm setting to python 2.7 now (and use addcleanup - but not just for that), since we have the repository. (There is concern that we test the code against python 2.7 and production runs 2.6 but we need to do this eventually anyway and current code is python 2.7 compatible. - One concern is Tier0 code which shares the same unittest strtucture. But I think it is not much to change Tier0 to make python 2.7 ready)

I'm not sure that we need to mix that.

Eric, here is abstract class which may fit the bill.
#!/usr/bin/env python
#-*- coding: utf-8 -*-
#pylint: disable=
"""
File       : WMCoreUnitTest.py
Author     : Valentin Kuznetsov <vkuznet AT gmail dot com>
Description: Base class for WMCore database related unit tests.
             It defines test database name based on user preferences specified via
             environment variables and setup logic to setup/tear down actions
             based on database.
"""
from __future__ import print_function

# system modules
import os
import sys
import unittest

class WMCoreUnitTest(unittest.TestCase):
    """
    Abstract class to implement proper setup/tear down methods for communicating
    with WMCore database back-end in unittests.
    """
    def __init__(self, methodName='runTest'):
        super(WMCoreUnitTest, self).__init__(methodName)
        self.dbName = os.environ.get('WMCORE_TEST_DATABASE', \
                'unittest_%s' % self.__class__.__name__)
        print("WMCoreTestDB test %s database" % self.dbName)
        enforce = os.environ.get('WMCORE_TEST_DATABASE_DELETE', False)
        if  enforce:
            print("WMCORE_TEST_DATABASE_DELETE=%s, will delete %s" % (enforce, self.dbName))
            self.deleteDatabase()
            self.createDatabase()

    def setUp(self):
        """Setup database for unittests"""
        if  self.dbName.startswith('unittest'):
            self.deleteDatabase()
            self.createDatabase()

    def tearDown(self):
        """Tear down database"""
        if self.dbName.startswith('unittest'):
            self.deleteDatabase()

    def createDatabase(self):
        """Abstract method to create test database, must be implemented in sub-classes"""
        print("Create %s" % self.dbName)

    def deleteDatabase(self):
        """Abstract method to delete test database, must be implemented in sub-classes"""
        print("Delete %s" % self.dbName)

And the logic I followed:

read database name from WMCORE_TEST_DATABASE environment, if it is not set define
database name wrt class name.
re-create database (i.e. delete and create) if its name starts with unittest
delete database in tear down if its name starts with unittest
define abstract methods to delete and create databases
use enforcement mechanism to re-create database ONCE in constructor of the class, this can be used
to address Andrew's worries about integration scenario. The database will be deleted
and created again only once for entire unittest (not for every method).

Then I foresee that we can make TestInit to inherit from this class and modify
its logic to use base class methods.
Best,
Valentin
On  0, Eric Vaandering<EMAIL_ADDRESS>wrote:

Let me be more clear on my proposal. I am not saying to leave this as it is forever. But I'm leery of big patches that change everything. This is especially difficult since we have another "big patch" outstanding and the two are bound to collide.
So what I would rather see, if possible, is a small patch that adds a new class that "does it right" and just a few tests that use it. Then we can work on migrating the rest until they are all done. That new class should really use inheritance to move as much of the "action" as possible into the new class.
I realize this probably makes TestInit (I think that was the code) a bit more complex in the mean time since it has to have methods to support the new way of doing things and the old, but IMO, this is a more deliberate, conservative way to approach this.
I realize what we have been doing may not be optimal, but it has worked for us, more or less, for several years. Taking 6 months to transition over is not going to kill us.
I'll come back to some of the more minor points next week.

Reply to this email directly or view it on GitHub:
https://github.com/dmwm/WMCore/pull/6260#issuecomment-141720739

For this plan to work we need to re-factor TestInit internals several times. In current implementation there are two methods: - all unit tests call setDatabaseConnection() in their setUp() methods therefore we need to implement your a/b/c steps over there - all unit tests call clearDatabase() in their tearDown() methods therefore we need to implement deletion of database over there I would assume that we'll create proper internal methods in TestInit which can be used in re-factoring I mentioned above. Then we'll need to re-factor TestInit again to replace setDatabaseConnection() and clearDatabase() with proper structure. Am I correct?

Yes, that is correct. But I think refactoring shouldn't be that much hassle.
As you said

create internal method to to do a/b/c. and
make those calls in side setDatabaseConnection method
change clearDatabase call to delete the database.

When all the test is done, we just need to change the name of setDatabaseConnection to proper name and replace all unittest. (Assuming a/b/c need to be called always together in set up).
The point I wanted to make is, if possible, we keep the changes only in TestInit initially.
Thanks,

This patch should only concentrate on the unittest setting. (For generator and fix for transaction and connection handling should be separate patches - each)

Here is PR for transaction logic: https://github.com/dmwm/WMCore/pull/6274
Once it is in place I can provide another patch on top of it to fix connection
issues. Then I can patch on top of that data structure logic, i.e. convert code
to use generators.

Here is PR for transaction logic: https://github.com/dmwm/WMCore/pull/6274 Once it is in place I can provide another patch on top of it to fix connection issues. Then I can patch on top of that data structure logic, i.e. convert code to use generators.

Thanks Valentin, Sorry I didn't know you have to create patch in sequence. The transaction and connection patch doesn't require other parts of code change but generator patch does, correct?
In that case, we can have one patch for transaction and connection (two commits will be fine) but generator patch separately. Transaction and connection patch won't be merged at least the cmsweb web validation is finished. (We can run the test after that, Is it OK with you?).

Commenting on you proposed subclass:
I don't get " use enforcement mechanism to re-create database ONCE in constructor of the class, this can be used
to address Andrew's worries about integration scenario. The database will be deleted
and created again only once for entire unittest (not for every method)." I think @PerilousApricot  concern was only with databases that were not left totally empty by the delete DAO. I don't understand how this helps that scenario.
Second question, why does deleting/creating the database have to be implemented in the subclass? Some or all of what needs to be done could be implemented in the base class, right?
Comment: we have some (many) unit tests who have nothing to do with underlying databases. Maybe they don't benefit at all from subclassing our own test case. If so, maybe just a name change from WMCoreUnitTest is appropriate.

On  0, ticoann<EMAIL_ADDRESS>wrote:

Here is PR for transaction logic: https://github.com/dmwm/WMCore/pull/6274 Once it is in place I can provide another patch on top of it to fix connection issues. Then I can patch on top of that data structure logic, i.e. convert code to use generators.

Thanks Valentin, Sorry I didn't know you have to create patch in sequence. The transaction and connection patch doesn't require other parts of code change but generator patch does, correct?

Since transaction, connection and improper usage of data structures/database
proxyresults objects are within processData() method fixing them
separately and merging those patches will cause conflict. That's why they
should go in sequence.  So either you accept sequence of patches or I can do
everything in one shot, see src/python/WMCore/Database/DBCore.py in
https://github.com/dmwm/WMCore/pull/6227/files PR.

I don't get " use enforcement mechanism to re-create database ONCE in constructor of the class, this can be used
to address Andrew's worries about integration scenario. The database will be deleted
and created again only once for entire unittest (not for every method)." I think @PerilousApricot  concern was only with databases that were not left totally empty by the delete DAO. I don't understand how this helps that scenario.

In current design, if I do

python setup.py test --buildBotMode=true --reallyDeleteMyDatabaseAfterEveryTest=true ...
and
python test/python/.../_t.py
are doing different things. In first case, database will be destroyed after
every test and it will be re-created. While this is not true in second case.  I
provided details that this is due to usage of globals and setup_test.py sets
the trashDatabases in TestInit.py This is why it is REALLY bad, since we can't
reproduce unittest behavior using 1 and 2 way to run unit tests.  Well, we
can make 2 to be identical to 1 if we don't use
reallyDeleteMyDatabaseAfterEveryTest flag, so we even have more complex
scenario.

In new design, using environment we will achieve that all aforementioned cases
will run in identical environment. For example, we set environment
and run 1 or 2 and they will deal with database in the same unique way
(whatever it is). Either database will be created for every
unit tests with unique name, or we'll use one common database for all tests.
And, we will have control to either delete database or not. But both 1
and 2 way to run unit tests will behave identically from database
deletion/creation point of view and regardless how we run unit tests (via
buildbot or not).
Regarding left overs from DAO. Starting from clean db is a way to go. Doing
this way we guarantee that we delete previous context and created fresh DB.  If
user need to understand why/what was left after previous test they can inspect
left-overs and decide if they want to keep them or not.  The control can be
done via setting environment rather through globals and flags.

Second question, why does deleting/creating the database have to be implemented in the subclass? Some or all of what needs to be done could be implemented in the base class, right?

it depends on how you want to design to go. It can be either be part of base
class or delegated to another level.

Comment: we have some (many) unit tests who have nothing to do with underlying databases. Maybe they don't benefit at all from subclassing our own test case. If so, maybe just a name change from WMCoreUnitTest is appropriate.

Then don't inherit from this class. Probably WMCoreUnitTest is not proper name
and should be WMCoreDatabaseUnitTest or similar.

@amaltaro , do we need to keep this open. It is already 7 years old and was done for py2 era.

@vkuznet you are the main contributor of this PR, if you are okay with closing this out, I am all in favor.
As you said, it's a 7 years old code proposal, with lots of discussion and good content, but if we haven't converged on this so far, it might be better to come back with a clean proposal when we deem such development necessary again.
It seems to be addressing many things in one PR as well. If you feel like there is something that must be changed, please try to isolate it and create a new GH issue.
I am closing this PR, please reopen it in case I am missing anything. Thank you all for your contribution and rich discussion!


Flyway Clean fails with ORA-32794 on Oracle

Which version and edition of Flyway are you using?
Flyway Community Edition 9.11.0
Which client are you using? (Command-line, Java API, Maven plugin, Gradle plugin)
Command-line via bash in VSCode on windows.
Which database are you using? (Type & version)
Oracle 19.1
Which operating system are you using?
Windows 10 enterprise 64 bit
What did you do? (Please include the content causing the issue, any relevant configuration settings, the SQL statement(s) that failed (if any), and the command you ran)
When creating a table with an identity primary key outside of flyway (using DBeaver 22.3.2) and then dropping it, I cannot clean the schema anymore without first purging the recycling bin.
in DBeaver:
CREATE TABLE tbl (
    col1              NUMBER GENERATED ALWAYS as IDENTITY(START with 1 INCREMENT by 1)        NOT NULL, CONSTRAINT tblpk PRIMARY KEY (col1)
)
;
DROP TABLE tbl;

Running flyway clean at this point errors out (see below).
Running PURGE RECYCLEBIN; in DBeaver makes flyway clean work fine again.
SELECT * FROM user_recyclebin shows the following:

OBJECT_NAME
ORIGINAL_NAME
OPERATION
TYPE
TS_NAME
CREATETIME
DROPTIME
DROPSCN
PARTITION_NAME
CAN_UNDROP
CAN_PURGE
RELATED
BASE_OBJECT
PURGE_OBJECT
SPACE

BIN$8oqBnAOiaBbgUwg3lAouEg==$0
TBLPK
DROP
INDEX
TS3A
2023-01-18:13:59:09
2023-01-18:13:59:10
6033340405379

NO
YES
902771
902771
902773
0

BIN$8oqBnAOjaBbgUwg3lAouEg==$0
TBL
DROP
TABLE
TS3A
2023-01-18:13:59:09
2023-01-18:13:59:10
6033340405379

YES
YES
902771
902771
902771
0

What did you expect to see?
$ flyway clean
Flyway is up to date
Flyway Community Edition 9.11.0 by Redgate
See what's new here: https://flywaydb.org/documentation/learnmore/releaseNotes#9.11.0

Database: jdbc:[redacted] (Oracle 19.1)
Successfully dropped pre-schema database level objects (execution time 00:00.001s)
Successfully cleaned schema "[redacted]" (execution time 00:00.109s)
Successfully cleaned schema "[redacted]" (execution time 00:00.039s)
Successfully dropped post-schema database level objects (execution time 00:00.000s)

What did you see instead?
When running flyway clean after dropping the table, I get the following error:
$ flyway clean
Flyway is up to date
Flyway Community Edition 9.11.0 by Redgate
See what's new here: https://flywaydb.org/documentation/learnmore/releaseNotes#9.11.0

Database: jdbc:[redacted] (Oracle 19.1)
Successfully dropped pre-schema database level objects (execution time 00:00.001s)
ERROR: Unable to clean schema "[redacted]"
------------------------------
SQL State  : 99999
Error Code : 32794
Message    : ORA-32794: cannot drop a system-generated sequence

Caused by: java.sql.SQLException: ORA-32794: cannot drop a system-generated sequence

Caused by: Error : 32794, Position : 22, Sql = DROP SEQUENCE "[redacted]"."ISEQ$$_902762", OriginalSql = DROP SEQUENCE "[redacted]"."ISEQ$$_902762", Error Msg = ORA-32794: cannot drop a system-generated sequence

Related issue: https://github.com/flyway/flyway/issues/3112

Also related: https://github.com/flyway/flyway/issues/1049
Apparently this was fixed previously

The issue here is to do with the RECYCLEBIN
We're unlikely to add purging the RECYCLEBIN to Flyway unless there's more interest/need, so you could PURGE RECYCLEBIN in a beforeClean callback to work around this, or disable the RECYCLEBIN as in https://github.com/flyway/flyway/issues/3112#issuecomment-801012756


Fold ci/kokoro/ubuntu into ci/kokoro/docker

In g-c-cpp-spanner we learned how to run the integration tests against production using the Docker builds. We should fold ci/kokoro/ubuntu to remove a build type, and make the builds more like g-c-cpp-common and g-c-cpp-spanner.

This was done a while back.


[CT-2809] Support ref in foreign key constraint expressions

originally left as comment in https://github.com/dbt-labs/dbt-core/issues/7417
I'm opening this issue to track upvotes/comments that could inform eventual prioritization. Is this something people want/need in their production workflows? Are happy to solve by other means in the meantime (e.g. dbt_constraints)?

If we were to take FK constraints more seriously, we're missing a pretty important ingredient, which is the ability to include & template ref inside the expression field — or providing more structure, i.e.
constraints:
  - type: foreign_key
    ref_table_name: ref('other_table_name')
    ref_column_names: ['id'] # could be multiple

Per https://github.com/dbt-labs/dbt-core/issues/6754#issuecomment-1449200569, we kicked that out of scope for v1.5, and we're unlikely to prioritize it while this remains a metadata-only (nonfunctional & unenforceable) feature on the majority of data platforms.

An argument in favor of prioritizing this is that BigQuery now supports the use of foreign keys for optimizing joins.
https://cloud.google.com/blog/products/data-analytics/join-optimizations-with-bigquery-primary-and-foreign-keys?hl=en

I would also submit that, database enforcement implementation aside, forcing the usage of explicit <schema>.<table> hardcodings and not supporting ref() is a crack in dbt's abstraction model. On its own it's certainly not the end of the world, but these breaks in the overall architectural vision and product conceptualization tend to proliferate if left unaddressed.

Snowflake can also use foreign keys for optimizing joins:
https://docs.snowflake.com/en/user-guide/join-elimination#setting-the-rely-constraint-property-to-eliminate-unnecessary-joins

I'd really be interested in referencing a FK constraint to a model that lives in a custom schema. The referred model lives in a custom schema that is dependent on an Environment Variable that is passed in at runtime, so I cannot hardcode a <schema>.<table> reference in my constraint as I do not know what it will be ahead of time.
Until dbt is enhanced to suport ref() in a foreign key constraint, I cannot model my FKs in constraints.

Another reason to add this is to ensure that dbt builds DAG dependencies that support the foreign keys.  Because there is no ref(), but instead the hard-coding specification of <schema.table>, there’s no way to for dbt to understand the DAG dependency that a foreign key constraint creates.
For example, let’s say I have 3 models:  A, B, and C
B depends on A.
So if I say dbt run -m +B  it will first build A, then B.
So far so good. Now, suppose I specify a foreign key constraint on a column in B, referring to a column in C.  For this to work, C has to exist. In other words, there’s now a DAG dependency between B and C, for that reason.
 The constraint itself causes an error, because C does not exist.
In any non-trivial sized DAG, this will cause constant errors in builds, because there is no guarantee of a thread getting to C before B.
The workaround is to force the dependency by placing a SQL-commented ref() in the model .sql, as described here. In other words, something like:
-- {{ ref('C') }}
But this is just more extra work, and it becomes difficult to maintain as it scales. So this is one more reason to support ref in foreign key constraints expressions in the .yml; i.e. all in the same place.

Like Snowflake and BigQuery, Redshift also uses foreign keys for optimizing joins:
https://docs.aws.amazon.com/redshift/latest/dg/c_best-practices-defining-constraints.html

During development we build into developer dependent datasets (e.g. dev_developer_name.dataset_name__model_name instead of dataset_name.model_name in production), so hard coding foreign keys seems impossible.

During development we build into developer dependent datasets (e.g. dev_developer_name.dataset_name__model_name instead of dataset_name.model_name in production), so hard coding foreign keys seems impossible.

@elyobo
The dependency issue raised by @noahjgreen295 will still be an issue and was a major issue for us in using this feature. Our pipelines were less reliable and there was essentially a race condition when running multiple models in parallel.
I use a similar naming convention to you and I used something like this in the model YAML
- type: foreign_key
  expression: "{{ 'warehouse' if target.name!='dev' else target.dataset }}.tableA(tableB_ForeignKey)"

you can define simple if-else logic in the brackets. This allows for the FKs to be created in a dev_developer_name schema under a dev target. Hope this helps!

Thanks @Stochastic-Squirrel, I didn't realise you could do that; ends up something like this for ours and does indeed work, leaving the logic duplication (this is already handled in the naming macros that ref calls) and the dependency issue.
          - type: foreign_key
            expression: "{{ 'prod_dataset.' if target.name!='dev' else target.dataset ~ '.prod_dataset__' }}foreign_table(foreign_key)"

Another option might be a post hooks alterations with alter table statements, but also not ideal. ref support would be ideal but can appreciate that it's a pain to implement.

@jtcohen6 Given that Snowflake, Redshift and BigQuery use foreign keys to optimize joins, will this issue get re-prioritized? Also, I'll add that downstream tools can use PK/FK to infer table relationships, perhaps bumping the priority further.

@Stochastic-Squirrel could you not just use target.schema?
- type: foreign_key
  expression: "{{target.schema}}.tableA(tableB_ForeignKey)"

Any updates on the priority for this? I feel like dbt focus a lot in adding new features but pushes aside the improvement of great features already present...

Any updates on this? It defeats the purpose of foreign key constraints as we cannot use them because it seems that dbt is unable to build a correct DAG. I have to run the project a couple of times so that parent tables get built.

+1 for this functionality
I am currently using this workaround successfully. Specifying this in the in-sql config block, in the post_hook argument:
{% if is_incremental == false and execute%}
ALTER TABLE {{this}} ADD FOREIGN KEY ('my_root_column') REFERENCES {{ref('my_foreign_model')}} ('my_foreign_column')
{% endif %}

This applies the constraint only when the model is materialized for the first time, avoiding unnecessary runs.

I would definitely appreciate a feature which fixes this open issue. I tried to implement the workaround below but found that my foreign table is built after the table I am defining the foreign key for. So, I had to go create the primary key for the foreign table manually in BigQuery in order for the production DAG to build properly.
- type: foreign_key   expression: "{{ 'prod_dataset.' if target.name!='dev' else target.dataset ~ '.prod_dataset__' }}foreign_table(foreign_key)"

Hello Team,
I checked this pull request, it does not state that "ref" are now supported in the "expression" for constraints (foregin_key).
It does not work in the following version, so is it planned to be fixed ?
dbt --version
using legacy validation callback
Core:

installed: 1.8.4
latest:    1.8.4 - Up to date!

Plugins:

databricks: 1.8.4 - Up to date!
spark:      1.8.0 - Up to date!

There has not been a release since before that PR was merged; did you check the code from that PR (which does show examples of ref() in a to option - it looks like expression has split into to (which takes the ref) and to_columns (the columns referenced in the to table) and if so did you use the new options, or are you using the older 1.8.4 release which doesn't have this change yet?

@elyobo thanks i tried out using to and to_columns with the 1.8.4 release, i got the following error message
Compilation Error in model customer_interactions (models/silver/customer_interactions.sql)
  No parent table defined for foreign key:

I will try it later today with the beta 1.9 version today.

Is this enabled for DBT Cloud as well?

This will be available in the 1.9 release of dbt-core!
If you're using dbt Cloud, you can access this early if you're running on versionless.
Documentation will be added soon -> https://github.com/dbt-labs/docs.getdbt.com/issues/5983


feat: example textbook

This is the beginning of a sample project that contains a textbook, based on the Verso manual genre.

While editing:

Rendered:


Inconsistency with authentication types and strange behaviour

Describe the bug
OIDC
When searching via Podcastindex, nothing happens, and checking the developer tools shows that it's returning a 401 Unauthorized error for the URL https://podfetch.domain.tld/api/v1/podcasts/1/search%20term/search. When adding an RSS feed manually, the same thing happens.
I also notice that if I go to the Administration page, I can't access anything because I'm not an administrator but there's no information on how to set an administrator or anything.
Basic Auth
When searching via Podcastindex, nothing happens, and checking the developer tools shows that it's returning a 502 Bad Gateway error for the URL https://podfetch.domain.tld/api/v1/podcasts/1/search%20term/search. When adding an RSS feed manually, the same happens except it does actually add it (even though it appears nothing happens). Trying to delete it with files causes a 502 and nothing happens, deleting without files works fine, although it doesn't update the UI until refreshed.
The Administration page works fine for this type.
No Auth
Essentially the same as Basic Auth but without the Administration page.

There doesn't seem to be any useful Docker logs so I'm not sure if I can find information anywhere as to what's happening.
Reproduction
See below
System Info
My compose file is as follows:
services:
  podfetch:
    container_name: podfetch
    image: samuel19982/podfetch:latest
    networks:
      main:
    ports:
      - "4635:8000"
    volumes:
      - /media/nas/media/podcasts:/app/podcasts
      - ./volumes/podfetch/database:/app/db
    environment:
      POLLING_INTERVAL: 60
      SERVER_URL: https://podfetch.${DOMAIN}

      # BASIC_AUTH: true
      # USERNAME: username
      # PASSWORD: password

      # OIDC_AUTH: true
      # OIDC_AUTHORITY: https://auth.${DOMAIN}/application/o/podfetch/
      # OIDC_CLIENT_ID: client_id
      # OIDC_REDIRECT_URI: https://podfetch.${DOMAIN}/ui/login/
      # OIDC_SCOPE: openid profile email
      # OIDC_JWKS: https://auth.${DOMAIN}/application/o/podfetch/jwks/

      PODINDEX_API_KEY: podindex_key
      PODINDEX_API_SECRET: podindex_secret
    restart: unless-stopped

    labels:
      traefik.enable: true

      traefik.http.routers.podfetch.rule: HOST(`podfetch.${DOMAIN}`)
      traefik.http.routers.podfetch.entrypoints: https
      traefik.http.routers.podfetch.service: podfetch
      traefik.http.services.podfetch.loadbalancer.server.port: 8000

and obviously uncommenting/commenting the BASIC_AUTH and OIDC_AUTH blocks depending on what I want to enable
If there's any other info that'll be helpful, let me know

Also, the output of the command is as follows:
System:
    OS: Linux 5.4 Ubuntu 20.04.6 LTS (Focal Fossa)
    CPU: (8) x64 Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
    Memory: 629.38 MB / 15.58 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 8.12.0 - ~/.nvm/versions/node/v8.12.0/bin/node
    npm: 6.4.1 - ~/.nvm/versions/node/v8.12.0/bin/npm
  Browsers:
    Firefox: 112.0.2

and that is on my server, with Docker version 20.10.17, build 100c701.
The browser I'm accessing PodFetch from is Firefox version 113.0.1
Used Package Manager
n/a
Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guide.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A, please open a GitHub Discussion instead.
[X] The provided reproduction is a minimal reproducible of the bug.

Thanks for creating an issue. You can find more about the CLI here. With that you can create a user that has the role admin. That should allow you access to the admin page. Concerning the other option I need to test again. I used Podindex a few days ago without a problem. Did you copy the keys and secrets correctly from the mail?

With that you can create a user that has the role admin. That should allow you access to the admin page

How would I do that for a docker container? Using docker exec podfetch "podfetch users --help" gives me a "podfetch": executable file not found in $PATH error.

Did you copy the keys and secrets correctly from the mail?

I did, but I noticed that the secret has a dollar sign in it, and docker-compose was trying to do variable substitution on it. Escaping the dollar sign fixed that issue, except with OIDC. With OIDC authentication, the 401 errors are still happening.

With that you can create a user that has the role admin. That should allow you access to the admin page

How would I do that for a docker container? Using docker exec podfetch "podfetch users --help" gives me a "podfetch": executable file not found in $PATH error.

Did you copy the keys and secrets correctly from the mail?

I did, but I noticed that the secret has a dollar sign in it, and docker-compose was trying to do variable substitution on it. Escaping the dollar sign fixed that issue, except with OIDC. With OIDC authentication, the 401 errors are still happening.

Oh I should mention that. Seems like dollar is common in their API keys. Thought I would be the only one who has a $ in his key.
Try docker exec -it root-podgrabv2-1 /app/podfetch users --help . Because I use a state of the art distroless image from google there is close to nothing in there except the app. That is why paths don't really work in there.

If you assign the admin account the 401 errors should vanish. There are essentially 3 types of users:
users => Can only listen, assign favorites
uploaders=>Can additionally  add podcasts
admin => Can do anything on the server including administrating users.

Perfect, that worked (although replacing root-podgrabv2-1 with my container's name)! It all seems to be working now, thanks for the help :)


EID-457: New eIDAS country/scheme picker

added local vars to .env to point to flags and scheme logos
new choose_a_country controller, html template and CSS stylesheet
country display decorator now adds schemes to countries
added display data, repository and decorator for eidas_scheme objetcts
added a method to yaml_loader to load files as a hash with filename as simple_id so we can map schemes to their logos
added a method to repository_factory to load display_data without translation as eIDAS scheme names don't need that

What's happening with the commits in this PR?  Seems to be about 9 stories.  Also it would be good to have some useful descriptive text in the summary.

I merged master into the branch and I guess git picked it up. But the changes detected are only the ones in my own commit..... go figure!
I've copied the description from my commit 👍


feat: add cleaner for Helm releases on Kubernetes v1.25

Issue: #39366

Problem
When upgrading a cluster from Kubernetes v1.24 to v1.25, if Helm installations containing PodSecurityPolicies exist a cluster operator is unable to upgrade or delete these installations. This happens because Helm keeps track of the resources applied prior to the cluster upgrade.

Solution
This PR aims to address that by including a process that will clean up releases in order to preserve a cluster operator ability to remove or upgrade those releases.
This process is part of the cluster reconciliation loop. Once a cluster is updated, a check is performed to see if: 1) the cluster has not undergone Helm release cleanup; and 2) the cluster is in Kubernetes v1.25. If both conditions are true, the cleanup process kicks off and removes any leftover PSPs. This process can also be extended to replace other superseded APIs as needed.
This process also adds a condition to the cluster, in order to keep track of previous runs of this cleanup process.

Testing

Engineering Testing
Manual Testing

Ran Rancher locally.
Created an external, cloud hosted RKE cluster with Kubernetes v1.24.
Installed rancher-logging on this cluster (contains PSPs).
Upgraded the cluster to Kubernetes v1.25.
Checked that the rancher-logging release was upgradeable/removable.

Test result: can successfully upgrade or delete the Helm release that previously contained PSPs.

Automated Testing

Added automated coverage for manifest replacement only.

QA Testing Considerations

These changes are mostly only valid for upgrades from Kubernetes v1.24 to v1.25.
Some scenarios that are worth testing:

Upgrade scenarios from Kubernetes v1.24 to v1.25:

On both local and downstream clusters.
With charts installed from a MacOS terminal, from a Linux terminal, from a Windows terminal (for verifying different kinds of line breaks).

Regressions Considerations

Is it necessary for this functionality to be wrapped in a feature flag? It is on by default so i imagine it is going to run automatically for most people anyway.

We had originally discussed having an option to disable it and I assumed the feature flag was the way to go. Happy to reconsider it though, or change to another, more suitable way of disabling it.

Closing this for now as upon discussion we have decided to use a different route.


How to switch to RGB camera with Javascript, WebRTC

Our website use plugin WebRTC to make a telepresence feature, so I use .getUserMedia() to take video from realsense as a webcam, but the default camera is the stereo module, how can I change to the RGB module ?
Please help me,
My cam is D453i

Hi @tantitnomashi,
Please take a look at this WebRTC sample that demonstrates how to select audio and video sources:
https://github.com/webrtc/samples/blob/gh-pages/src/content/devices/input-output/js/main.js
Thank you!

Hi @tantitnomashi,
Do you need any further clarifications regarding this topic?


[4.0.0-beta.5] virtual-scroll no render

Bug Report
Ionic Info
Ionic:
   ionic (Ionic CLI)          : 4.1.1 (/usr/local/lib/node_modules/ionic)
   Ionic Framework            : @ionic/angular 4.0.0-beta.5
   @angular-devkit/core       : 0.7.0-rc.3
   @angular-devkit/schematics : 0.7.0-rc.3
   @angular/cli               : 6.1.0
   @ionic/ng-toolkit          : 1.0.0
   @ionic/schematics-angular  : 1.0.1

Cordova:

   cordova (Cordova CLI) : 7.1.0
   Cordova Platforms     : browser 5.0.4, ios 4.5.5
   Cordova Plugins       : cordova-plugin-ionic-keyboard 2.1.2, cordova-plugin-ionic-webview 2.0.2, (and 7 other plugins)

System:

   ios-deploy : 1.9.2
   ios-sim    : 6.1.1
   NodeJS     : v10.6.0 (/usr/local/bin/node)
   npm        : 6.4.0
   OS         : macOS High Sierra
   Xcode      : Xcode 9.4.1 Build version 9F2000

Describe the Bug
I have just update ionic core version to 4.0.0-beta.5 and my virtual-scroll doesn't render anything.
this is my page html:
<ion-content>
    <my-loader *ngIf="loading"></my-loader>
    <ion-virtual-scroll *ngIf="!loading" [items]="stations" approxItemHeight="45px">
        <ion-item button *virtualItem="let station" (click)="itemSelected(station)">
            {{station.name}}
        </ion-item>
    </ion-virtual-scroll>
</ion-content>

Additional Context
this is my package.json
"dependencies": {
    "@angular/common": "6.1.0",
    "@angular/core": "6.1.0",
    "@angular/forms": "6.1.0",
    "@angular/http": "6.1.0",
    "@angular/platform-browser": "6.1.0",
    "@angular/platform-browser-dynamic": "6.1.0",
    "@angular/router": "6.1.0",
    "@ionic-native/core": "5.0.0-beta.14",
    "@ionic-native/geolocation": "5.0.0-beta.14",
    "@ionic-native/globalization": "5.0.0-beta.14",
    "@ionic-native/google-maps": "^5.0.0-beta.20",
    "@ionic-native/splash-screen": "5.0.0-beta.14",
    "@ionic-native/status-bar": "5.0.0-beta.14",
    "@ionic/angular": "4.0.0-beta.5",
    "@ionic/ng-toolkit": "1.0.0",
    "@ionic/pro": "2.0.3",
    "@ionic/schematics-angular": "1.0.1",
    "@ngx-translate/core": "^10.0.2",
    "@ngx-translate/http-loader": "^3.0.1",
    "angularfire2": "^5.0.0-rc.11",
    "cordova-browser": "5.0.4",
    "cordova-ios": "4.5.5",
    "cordova-plugin-device": "^2.0.2",
    "cordova-plugin-geolocation": "^4.0.1",
    "cordova-plugin-globalization": "^1.11.0",
    "cordova-plugin-googlemaps": "^2.4.1",
    "cordova-plugin-ionic-keyboard": "^2.1.2",
    "cordova-plugin-ionic-webview": "^2.0.2",
    "cordova-plugin-splashscreen": "^5.0.2",
    "cordova-plugin-whitelist": "^1.3.3",
    "core-js": "^2.5.3",
    "firebase": "^5.3.0",
    "ionic-plugin-keyboard": "^2.2.1",
    "lodash": "^4.17.10",
    "moment": "^2.22.2",
    "rxjs": "6.2.2",
    "zone.js": "^0.8.26"
  },
  "devDependencies": {
    "@angular/cli": "6.1.0",
    "@angular/compiler": "6.1.0",
    "@angular/compiler-cli": "6.1.0",
    "@angular/language-service": "6.1.0",
    "@angular-devkit/architect": "0.7.0-rc.3",
    "@angular-devkit/build-angular": "0.7.0-rc.3",
    "@angular-devkit/core": "0.7.0-rc.3",
    "@angular-devkit/schematics": "0.7.0-rc.3",
    "@types/jasmine": "~2.8.6",
    "@types/jasminewd2": "~2.0.3",
    "@types/node": "~10.5.2",
    "codelyzer": "~4.4.2",
    "jasmine-core": "~2.99.1",
    "jasmine-spec-reporter": "~4.2.1",
    "karma": "~2.0.2",
    "karma-chrome-launcher": "~2.2.0",
    "karma-coverage-istanbul-reporter": "~2.0.0",
    "karma-jasmine": "~1.1.1",
    "karma-jasmine-html-reporter": "^0.2.2",
    "protractor": "~5.4.0",
    "ts-node": "~7.0.0",
    "tslint": "~5.11.0",
    "typescript": "~2.9.2"
  },
  "description": "An Ionic project",
  "cordova": {
    "plugins": {
      "cordova-plugin-whitelist": {},
      "cordova-plugin-device": {},
      "cordova-plugin-splashscreen": {},
      "cordova-plugin-ionic-webview": {},
      "cordova-plugin-ionic-keyboard": {},
      "cordova-plugin-globalization": {},
      "ionic-plugin-keyboard": {},
      "cordova-plugin-geolocation": {
        "GEOLOCATION_USAGE_DESCRIPTION": "To locate you"
      },
    },
    "platforms": [
      "ios",
      "browser"
    ]
  }

I have the same issue and have had to revert to beta.3 for the time being.  Of course this suffers from other issues like the scroller not updating when it's data changes etc.  I think it would be useful to make some of the basic methods to update or recalculate the scroller public, so that we can force these updates ourselves.


feat(ci): Add codecov

Overview
Adds codecov to CI

#233  👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

 Codecov will compare coverage reports and display results in all future pull requests.
Thanks for integrating Codecov - We've got you covered :open_umbrella:


Document running tests locally. Fix #468.

Document running tests locally, leveraging docker.
act can be used too but may be be longer or more complex, to be tested more.

LGTM! One possible addition is to specify how to run the super-linter locally

I've added a short section on this. Never tried it much, apart with act and it seems there were some issues, maybe as I wasn't using the big container image that is 20GB, only the smaller one. To be tested.