Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Added get_url to handle authentication #681

Merged
merged 13 commits into from
Nov 25, 2018

Conversation

cezmunsta
Copy link
Contributor

Fixes #680

Copy link
Collaborator

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

Thank you for this!

May I ask for the following changes:

  • Extract "curl" outside the function. so that we do:
leader_check=$(curl $(get_curl_auth_params) -m 1 -s -o /dev/null -w "%{http_code}" "${api}/leader-check")
  • Only compute the function once, and not per invocation?

@shlomi-noach
Copy link
Collaborator

Also, can you please take into account the change in #675?

@cezmunsta
Copy link
Contributor Author

# ./orchestrator-client -b orchestrator:xxx -c all-instances
10.0.0.20:3306

# ORCHESTRATOR_AUTH_USER=orchestrator ORCHESTRATOR_AUTH_PASSWORD=xxx ./orchestrator-client -c all-instances
10.0.0.2:3306

# ./orchestrator-client -c all-instances                                                                                                              
parse error: Invalid numeric literal at line 1, column 4
^C^

# edit /etc/profile.d/orchestrator-client.sh

# cut -f1 -d= /etc/profile.d/orchestrator-client.sh
ORCHESTRATOR_API

# ./orchestrator-client -c all-instances
10.0.0.2:3306

@cezmunsta
Copy link
Contributor Author

# ./orchestrator-client -c all-instances
orchestrator-client[26762]: Cannot access orchestrator at http://localhost:3000/api.  Check ORCHESTRATOR_API is configured correctly and orchestrator is running

The value of curl_auth_params is tested against 401 Unauthorized and fails before attempting to execute curl ... instead of erroring on jq

@cezmunsta
Copy link
Contributor Author

@shlomi-noach OK now?

@shlomi-noach
Copy link
Collaborator

@cezmunsta this will unfortunately fail when orchestrator_api contains multiple entries, as per https://github.com/github/orchestrator/pull/681/files#diff-79f20b792813786c70a731862c24143eL157

@shlomi-noach
Copy link
Collaborator

As @cezmunsta clarified elsewhere, the check will not fail when orchestrator_api contains multiple entries.

@shlomi-noach shlomi-noach merged commit 686d60c into openark:master Nov 25, 2018
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