Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Do not call dssecrets unless it is installed #23

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Conversation

jesse-ross
Copy link
Contributor

Motivation

We would like to minimize the use of dssecrets, because it is so brittle, and to that end we have updated the data release docs to encourage people not to depend on it. However, when dssecrets is not installed, the scipiper pipeline gives a rather inscrutable error if the user is not logged in to ScienceBase.

Changes

This simply checks to see whether dssecrets is installed before calling it.

  • If dssecrets is installed, then it attempts to log in using it. It also adds a message saying it is doing so.
  • If dssecrets is not installed, then the pipeline will stop, and tell the user they need to authenticate and try again.

@msleckman
Copy link
Collaborator

msleckman commented Mar 30, 2023

I like this change and I like that it is explicit with the user via messages! As I mentioned earlier, in some cases, particularly when the session created with sbtools::authenticate_sb() expires because the scipiper pipeline takes a while to build, dssecrets is needed.
I foresee a potential issue here in the case when user does not have dssecrets installed and the scipiper pipeline is heavy enough that authenticate_sb() will be expire by the time the function using run in the pipeline. This require a bit further testing however and can be addressed as a separate PR as well.

@ted80810
Copy link

I copied over the changes the DRB estuary data release repo and ran scmake() once my session expired. I used sbtools::is_logged_in() to see if my session was still active. Below is the resulting error message:
image
Would 'dssecrets' be a dependence for another library in the list of packages at the top of the remake.yml file?

@jesse-ross
Copy link
Contributor Author

I foresee a potential issue here in the case when user does not have dssecrets installed and the scipiper pipeline is heavy enough that authenticate_sb() will be expire by the time the function using run in the pipeline. This require a bit further testing however and can be addressed as a separate PR as well.

Good call! We can maybe explore writing a little something to automate this - i.e. cache user-entered credentials locally and just re-enter them as needed. But I agree that that could be separated out from this work.

@jesse-ross
Copy link
Contributor Author

Would 'dssecrets' be a dependence for another library in the list of packages at the top of the remake.yml file?

The warnings about dssecrets are coming from the require() statements I put in. I am using that function for its return value, but it also is a bit wordy. I pushed up a quick change to silence the warnings.

The warning about SB authentication being required isn't coming from my code, but it seems satisfactory! So I removed the lines in my code that were meant to provide such a warning. Would you mind testing again, please? Hopefully the pipeline should fail in a more legible way now, i.e. just give the message about how you're not logged in.

@ted80810
Copy link

ted80810 commented Mar 30, 2023

yea that worked. Now, I just get the error message that the item does not exist or requires authentication:
image

@jesse-ross
Copy link
Contributor Author

OK, cool! I think this is actually close to something that we could merge. I'd love to have someone test it with dssecrets just to make sure that we haven't broken that experience. @msleckman @amsnyder does either of you have a pipeline handy that you could use to do this?

Note that while I was in here, I also added a diagnostic line to sb_secret_login() itself, saying when the secret's timestamp is, so that line would also have to be added in your version. The purpose is to give folks a hint if their dssecrets installation has gotten out of date, which is one of the many aspects of dssecrets that causes confusion 😂

@jesse-ross jesse-ross changed the title Draft: do not call dssecrets unless it is installed Do not call dssecrets unless it is installed Mar 30, 2023
@amsnyder
Copy link
Collaborator

I just tested and it works for me. I also added in the sb_replace_files_log function mentioned in #21 just to be efficient here. Let's make sure we apply these changes to here as well: https://github.com/USGS-R/hierarchical-data-release-template

@jesse-ross
Copy link
Contributor Author

Let's make sure we apply these changes to here as well: https://github.com/USGS-R/hierarchical-data-release-template

I'm not too practiced with templates. That looks like a template which is generated from this template, right? Does it need to be regenerated from this template, or would the same changes be applied to it separately?

amsnyder added a commit to USGS-R/hierarchical-data-release-template that referenced this pull request Mar 30, 2023
@amsnyder
Copy link
Collaborator

I'm not sure if there's a way to do that...I think we are just managing them as separate codebases at this point. I just opened a PR and tagged you as a reviewer: USGS-R/hierarchical-data-release-template#11

I think both can be merged at your discretion @jesse-ross

@jesse-ross jesse-ross merged commit c80b7e9 into main Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants