search

NASA-PDS / doi-service

Service and tools for generating DOIs for PDS bundles, collections, and data sets
https://nasa-pds.github.io/doi-service
Other
2 stars 3 forks source link

Config file changes #372

Closed alexdunnjpl closed 1 year ago

alexdunnjpl commented 1 year ago

🗒️ Summary

Overhauls configuration loading behaviour.

Previously, configurations were loaded from a handful of candidate locations which weren't robustly defined. Now, there is a default config in the source code, and an optional location (matching that given in the doco) pulled from sys.prefix whose keys will override default values. Environment variables will override both.

Default credential values for DataCite have been removed as these are a potential source of developer/user confusion. If these values are read without being set in the user config or env vars, a descriptive exception will be thrown.

Adds comprehensive tests for DOIConfigUtil to prevent similar issues in future

⚙️ Test Data and/or Report

Unit tests pass locally. Tested as wheel installation (where #371 arises) manually for a few actions, successfully.

♻️ Related Issues

Fixes #371 (probably. 99%. unit tests aren't set up to be run against a wheel installation, but I've run a few manual tests)

alexdunnjpl commented 1 year ago

Need to sort out failing BIT. I'm fuzzy on how the DataCite credentials are/were passed into the test environment, but I'll take a look at updating that tomorrow to supply them as env vars.

PR is open

alexdunnjpl commented 1 year ago

Depends on https://github.com/NASA-PDS/doi-service/pull/373

alexdunnjpl commented 1 year ago

Approved with the assurance that OSTI_USER and OSTI_PASSWORD env vars still work

They won't (specifically, I removed their insertion into the config in #373 ), but my understanding is that OSTI is deprecated/obsolete. I never had to provide OSTI credentials when running the tests locally so I'd assumed they wouldn't be necessary for the Action.

Anything there jump out at you as questionable @nutjob4life ?

nutjob4life commented 1 year ago

Approved with the assurance that OSTI_USER and OSTI_PASSWORD env vars still work

They won't (specifically, I removed their insertion into the config in #373 ), but my understanding is that OSTI is deprecated/obsolete.

Then these files will need to be updated as well:

Since the PDS Exposition instance of the DOI service is using OSTI, right @jordanpadams?

jordanpadams commented 1 year ago

@nutjob4life @alexdunnjpl no services should be using OSTI anymore. we no longer have access to their services.

alexdunnjpl commented 1 year ago

@nutjob4life on that basis, I'd cautiously suggest that we should be good to go - while you're correct that those files should be changed, I'd argue that should be part of complete OSTI excision (which we're waiting on @tloubrieu-jpl 's return to consider).

Basically the env vars' presence won't hurt anything, and I'd prefer not to touch anything unnecessary as I'm trying to sneak this in after the deadline for the current release.

alexdunnjpl commented 1 year ago

Rebased onto #373

nutjob4life commented 1 year ago

@nutjob4life on that basis, I'd cautiously suggest that we should be good to go

It also helps demonstrate that no one uses the PDS Exposition DOI Service, given the lack of working OSTI credentials and zero complaints! 🥲

Merge whenever you're ready!