Skip to content
This repository was archived by the owner on Jun 16, 2021. It is now read-only.

Update to use API token, instead of API key#2

Open
ywei2017 wants to merge 1 commit intoubidots:masterfrom
ywei2017:master
Open

Update to use API token, instead of API key#2
ywei2017 wants to merge 1 commit intoubidots:masterfrom
ywei2017:master

Conversation

@ywei2017
Copy link
Copy Markdown

Updated the endpoint URL, and use API token, instead of API Key.

Copy link
Copy Markdown

@d4vsanchez d4vsanchez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ywei2017,

Thanks for your contribution.

Unfortunately I'm not able to merge this PR for the following reasons:

  1. Even though we've basically stopped using the API Key for our public examples and kept using the tokens. This PR will change the way the whole Client.auth() method works by receiving the token instead of receiving the API Key. We could do that, but the version should be bumped to a major instead of a patch, as it currently has been done in this PR.

  2. It actually breaks the tests of some methods that depends on calling the Client.auth() method and getting the token after authenticating with the /api/v1.6/auth/token endpoint.

@ywei2017
Copy link
Copy Markdown
Author

@d4vsanchez , thanks for the quick feedback.

I was struggling with the Client.auth() method quite a bit as well.

  • On one hand, it seems to be obsolete since it's unnecessary with an API token.
  • Then I was also not sure, given the /api/v1.6 endpoint uses the same version as before. I acknowledge that I didn't test with an API key.

I also agree that moving away from the Client.auth() will be a major version change, and I don't know enough of the library usage to judge whether that is a good idea.

What would you suggest as next step? I did try to run some of the tests, but I got errors as well. If you can let me know the proper setting to run the tests, I can work more on it to make sure the tests are passing.

Thanks

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.

2 participants