-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use socrata connector, auth for ingest/library calls #1312
Conversation
00b5419
to
73bc340
Compare
73bc340
to
f495074
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1312 +/- ##
==========================================
+ Coverage 70.41% 70.51% +0.09%
==========================================
Files 114 115 +1
Lines 6129 6129
Branches 700 700
==========================================
+ Hits 4316 4322 +6
+ Misses 1669 1662 -7
- Partials 144 145 +1 ☔ View full report in Codecov by Sentry. |
dcpy/connectors/socrata/__init__.py
Outdated
SOCRATA_PASSWORD = os.getenv("SOCRATA_PASSWORD") | ||
|
||
|
||
def _simple_auth(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this and _socrata_request()
to some utils ile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
LGTM! The only rec is to move helper functions out of the init file |
f495074
to
05461a3
Compare
SOCRATA_PASSWORD = os.getenv("SOCRATA_PASSWORD") | ||
|
||
|
||
def _simple_auth() -> tuple[str, str] | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think we'd want to throw an exception if _simple_auth() is called without these vars present. Any reason not to? (IIRC the Socrata request would fail in that case, even for public resources)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going with no auth is fine for get requests for things like metadata or pulling datasets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to try to use auth when user/pass is defined, and let failures occur when a resource isn't allowed for a given user (or None
auth).
Arguably then, maybe we only call simple_auth if those are defined. But I thought this was simple - get optional auth credentials if available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, so you've tested that this works for public resources when there are no creds defined? (for some reason I remember that failing, but that was a long time ago.) Maybe it makes sense to just log a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - just ran locally with no issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool - had one question, but this looks good
05461a3
to
c589be0
Compare
Meant to close #1311
Seems that some dot datasets went private, meaning we can't simply download them without auth
2 commits just to make changes easier to see (1 just moves some logic, 2 makes needed changes)
passing run here