Skip to content
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

APIDataSet ease of use #1101

Closed
ianwhale opened this issue Dec 7, 2021 · 1 comment · Fixed by #1105
Closed

APIDataSet ease of use #1101

ianwhale opened this issue Dec 7, 2021 · 1 comment · Fixed by #1105
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@ianwhale
Copy link
Contributor

ianwhale commented Dec 7, 2021

Description

Howdy team!

I was working with the APIDataSet recently and had two issues out of the box.

1. Specifying the auth keyword argument in yaml

The requests library expects the auth parameter of a request to be either a HTTPBasicAuth or a tuple (lists are not allowed, see here in requests). At the moment, neither are possible to specify in my catalog.yml.

From what I hear, you're already working on this (#1011). So maybe this point is moot.

2. The auth keyword argument and credentials.yml

I would like to specify my (username, password) tuple inside credentials.yml. However, the APIDataSet's auth keyword wouldn't get filled in by the config loader.

To get this working, you'd have to extend APIDataSet to have a credentials keyword that is filled in for auth in an upcall.

It would be great to either have this by default, or even have the loader fill auth keywords in addition to credentials. Although that might have unintended consequences.

Context

Hopefully this would unify the experience a bit. Right now, the credentials keyword in a dataset and credentials.yml are the main points of access to secrets. Which is probably good.

Possible Implementation

I whipped up my own APIDataSet to solve both the problems above.

Possible Alternatives

To get this working with no changes to APIDataSet, we'd have to implement the changes in #1011 so we can specify tuples in credentials.yml and have the config loader fill in auth as well.

@ianwhale ianwhale added the Issue: Feature Request New feature or improvement to existing feature label Dec 7, 2021
@antonymilne
Copy link
Contributor

Hi @ianwhale, thank you for your very thoughtful write up of this. The points you raise are very fair. I'd suggest just implementing the changes you propose to APIDataSet right away as it will give an instant improvement, seems pretty uncontroversial and I like the code exactly how you wrote it 👍

For the possible alternative: #1011 is still very much up for discussion and would be a larger change. And auth is only relevant to the APIDataSet so I'd be inclined to leave the config loader alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants