Update README re: environment variables

[cjwetherington]

Updating README to reflect that a value for SENTRY_DSN is required. When getting this running locally (sans Docker), I encountered a fatal error indicating that this environment variable was unset. After reviewing harvester/config.py l.17, it became clear that it's required, and l.57 of same indicates "none" needs to be used for opt-out.

Cheers for this great OAI tool.

[ghukill]

Hey @cjwetherington, thanks for opening a PR! Glad you're finding this CLI app potentially useful.

Can confirm that without SENTRY_DSN set, an error is thrown, where even this --help call is enough to trigger the error during configuration:

pipenv run oai --verbose \
-o /tmp/test.xml \
-h http://example.com \
harvest --help

We chatted very briefly internally and concluded that we also don't find it ideal that Sentry is strictly required (or explicitly not used via SENTRY_DSN=none).

As such, would like to propose an alternative that makes even the presence of SENTRY_DSN optional. If we were to move SENTRY_DSN from REQUIRED_ENV_VARS to OPTIONAL_ENV_VARS here, that should support omitting that environment variable altogether.

Would that work for your purposes? It aligns with our preferred ways of working with the application as well. Let us know if you'd like to explore that option via a commit in this PR, or we could push a commit to this PR, etc.

[cjwetherington]

Hello there, @ghukill! Thanks for your very speedy reply. In retrospect, I probably should have just opened an issue rather than proposing revised docs, so apologies if I made things more complex than they need to be.

I quickly swapped SENTRY_DSN from the required to the optional set, but that seems to have triggered some sort of downstream parsing error:

I haven't got time at the moment to play with it much more than that, so feel free to add a commit here if you like, or even close this if the doc update won't be needed—otherwise I can have a peek sometime later this week and see if I can get it sorted. Thanks!

[cjwetherington]

Never mind; it occurred to me while walking back from lunch that it was just a problem of notation (trailing comma needed). I pushed that and revised the README accordingly. Let me know if there's anything else. : )

[ghukill]

Never mind; it occurred to me while walking back from lunch that it was just a problem of notation (trailing comma needed). I pushed that and revised the README accordingly. Let me know if there's anything else. : )

The 'ole trailing comma. Nice catch.

Changes look great, and align with our usage of the application as well. Just waiting on some internal review, but will hopefully merge (and tag a new release) fairly quick here.

Thanks again for the PR and reaching out!

[cjwetherington]

@ghukill My distinct pleasure. Just starting to use this to test and debug our OJS metadata endpoints, and it was the best tool I found by far, so kudos to you and team, and if I happen to come upon any other snags, I'll file an issue.

[ghukill]

Hey @cjwetherington - sorry for the churn here, but we're opting to close this PR in favor of one created by us internally, with the exact changes you have provided here. We need to solve for some CI/CD interactions with PRs opened from folks outside the library, and don't want to hold up this update.

Thanks again! That new PR should be merged shortly.