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

fix: Switch to discovery api for mode spaces #481

Merged

Conversation

youngyjd
Copy link
Member

@youngyjd youngyjd commented Apr 22, 2021

Getting spaces from Mode main API always hang and sometimes errorred out. We were suggested by Mode to integrate to the new Discovery API for getting Mode spaces

@youngyjd youngyjd changed the title Switch to discovery api for mode spaces fix: Switch to discovery api for mode spaces Apr 22, 2021
Signed-off-by: Junda Yang <youngyjd@gmail.com>
Signed-off-by: Junda Yang <youngyjd@gmail.com>
Signed-off-by: Junda Yang <youngyjd@gmail.com>
@youngyjd youngyjd force-pushed the switch-to-discovery-api-for-mode-spaces branch from e981b8d to 467e961 Compare April 22, 2021 18:42
# Spaces
params = {'auth': HTTPBasicAuth(conf.get_string(MODE_ACCESS_TOKEN),
conf.get_string(MODE_PASSWORD_TOKEN))}
# mode_bearer_token must be provided in the conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different from the normal api token? If so, could you document it in the config?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mode provides two kind of authentication. discovery api requires bearer token while the main api requires user password. see

def get_auth_params(conf: ConfigTree, discover_auth: bool = False) -> Dict[str, Any]:

Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the change to new mode API

@feng-tao
Copy link
Member

I think we should eventually change all the existing mode API to use the new discovery API

@feng-tao feng-tao merged commit c0557a5 into amundsen-io:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants