deprecate `connect_to_etn()` instead of not exporting

PietrH commented 2 months ago

connect_to_etn() is unused in v2.3
it should be deprecated instead of removed
and return a lifecycle warning similar to #301

TODO

[x] reexport connect_to_etn()
[x] Set return value of connect_to_etn() to NULL
[x] all examples should work again
[x] all vignettes should work again
[x] add test connect_to_etn(): should provide deprecation message
[x] Check if information in NEWS is still correct
[x] add deprecation badge to connect_to_etn()
[x] create new helper in place of connect_to_etn()
[x] create test for new helper
[x] implement use of helper in all sql helper functions
[ ] fix issue tests connect_to_etn() only fail in test package on RStudio VLIZ

All examples work again, but they still need to be updated: #308

I'm having a strange issue where the new tests for the deprecation message for connect_to_etn() fail on the RStudio VLIZ server, but only when I call test package or devtools::test(filter = "connect_to_etn"), not when running the tests directly or clicking run tests. Very strange. I also can't replicate it locally. Visually the warning message looks fine.

PietrH commented 2 months ago

I suppose connect_to_etn() should remain functional as we are now going down the road of soft deprecation

peterdesmet commented 2 months ago

Yeah, I guess:

The function remains and is marked deprecated
The function returns NULL to not leak credentials?

PietrH commented 2 months ago

Soft or hard deprecation

At the moment, the SQL versions of the functions themselves use connect_to_etn() to create database connections like so:

connection <- do.call(connect_to_etn, get_credentials())

So:

Soft deprecate and keep using internally: it'll keep working but is not encouraged
Hard deprecate, return NULL, add new helper. This should be easy enough.

What do you prefer?

Leaking credentials

Credentials are can be leaked via RHistory or scripts because people are encouraged to enter their password directly:

connect_to_etn(username = Sys.getenv("userid"), password = Sys.getenv("pwd"))

I'm currently still storing credentials as environmental variables, just like rgbif, but i'd prefer to use something like keyring in the future. This is why the new functions prompt the user, instead of allowing them to enter directly. I'm also using a dependency (askpass) for the password, using the RStudio API so this vital secret can't end up captured somewhere where it doesn't belong.

Any root/sudo user, or any child processes of the current user can access these credentials. It's non trivial for other users to get to them, but environmental variables still aren't a good place to store passwords in my opinion.

peterdesmet commented 2 months ago

Users that currently experience:

library(etn)
con <- connect_to_etn()
con
#> <OdbcConnection> peter.desmet@inbo.be@pgetn.vliz.be
#>   Database: ETN
#>   PostgreSQL Version: 14.0.13
animals <- get_animals(con, animal_id = 305)

Should get something like:

library(etn)
con <- connect_to_etn()
#> Deprecation warning:
#> connect_to_etn() has been deprecated since etn 2.3.0.
#> You will be prompted for credentials instead.
con
#> NULL
animals <- get_animals(con, animal_id = 305)
#> Deprecation warning:
#> `connection` has been deprecated since etn 2.3.0.
#> You will be prompted for credentials instead.

So I prefer:

Hard deprecate, return NULL, add new helper.

This will also make it clearer to us developers to no longer rely on connect_to_etn().

PietrH commented 2 months ago

I agree, that seems to be the cleanest way forward!

inbo / etn