This New branch is building upon the sqlite_test PR as it passed the testsuite#13
This New branch is building upon the sqlite_test PR as it passed the testsuite#13Farreeda wants to merge 58 commits intoJuliaDatabases:devfrom
Conversation
fixing the file path issue by identifying its type to String - testing it(Passed)
removed .vscode folder added
testing the github secrets
TheCedarPrince
left a comment
TheCedarPrince
left a comment
There was a problem hiding this comment.
I think this is close with GitHub Actions being nearly done. That said, the tutorials look great and the tests look solid. I think the biggest issue is just connecting to the server. Otherwise, great work. Let me know if you have any questions or issues.
There was a problem hiding this comment.
Had to change the syntax to get this to re-trigger but it is now working.
There was a problem hiding this comment.
Updated authors and compat entries.
There was a problem hiding this comment.
I overhauled this a little to deprecate some support for ODBC and JDBC connections as well as add a proper import.
There was a problem hiding this comment.
I wasn't able to get tests for thsi working so I am not sure how well this works.
There was a problem hiding this comment.
I know we were thinking that your function _dbconnect could have kwargs, but I think I'd rather have the user have to define their own kws to handle the specifics of the connection string like in the original _dbconnect that I brought back. What do you think @Farreeda ?
There was a problem hiding this comment.
Also, I was able to test the old version and that does work on my computer to run tests but I couldn't get the new dispatch working that you had created.
There was a problem hiding this comment.
If that means adding kwargs in addition to arguments, this is a good idea. No connection can happen without the identified arguments as user, password, host, do you agree @TheCedarPrince ?
which dispatch?
There was a problem hiding this comment.
This works perfectly!
There was a problem hiding this comment.
Added required test dependencies.
There was a problem hiding this comment.
I could not get MySQL tests working on the GitHub action but I know it is due to the connection string... Could you attempt debugging this?
test/runtests.jl
Outdated
| @testset "_dbconnect function for LibPQ" begin | ||
|
|
||
| conn= DBConnector._dbconnect(LibPQ.Connection, host = ENV["POSTGRES_HOST"], user = ENV["POSTGRES_USER"], dbname = "mimic", password = ENV["POSTGRES_PASSWORD"]) | ||
| @test @isdefined conn | ||
|
|
||
| end |
There was a problem hiding this comment.
Could you make sure the tests work? I think the host string needs to be formatted again or something... Not entirely sure. What you can do for testing is try connecting to the database on the server from your local machine and establishing how the connection string should work.
test/runtests.jl
Outdated
| #conn= DBConnector._dbconnect(LibPQ.Connection, ENV["POSTGRES_HOST"],ENV["POSTGRES_USER"], ENV["POSTGRES_PASSWORD"], "omop") | ||
| conn = DBConnector._dbconnect(LibPQ.Connection, "199.180.155.65", "postgres", "postgres3", "omop") | ||
| @test @isdefined conn |
There was a problem hiding this comment.
Hey Fareeda, could you change these strings to the appropriate GitHub Environment variables?
|
Hey @Farreeda, once you are done with the last comment I made to make sure the LibPQ tests still work even with GitHub environment variables, I think we can go ahead and merge this even though we don't have the test suite set-up for the MySQL server. Thanks! |
Almost the secrets issue is because the PR has no access to the environment secrets of the base repo
2 participants
No description provided.