feat(api/locate): add authorization header #231
+78
−24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This seems the cleanest and less frictious way to enable the m-lab/ndt7-client-go codebase to supply a token for the purpose of identifying the client registration. Technically speaking, this change is not extending the locate client API for supporting /priority/v2/nearest requests rather it just adds the ability of providing a token. (I do not know who's using this API but I can imagine an organization having their own API requiring a token, though I think it would be useful to clarify what is our user story here.) As a side effect, m-lab/ndt7-client-go would then choose the right URL and provide a token, still invoking Nearest. I tried other approaches but the fact that (1) the default base URL includes the /v2/nearest path, (2) the default base URL could be configured using CLI flags and (3) it is not clear to me who are the users of this API prompted me to proceed more cautiously and just make it possible to provide a token. Ideally, more in the long term, I think it would be useful to confirm the following: 1. who are the users of m-lab/locate/api/client and whether it is possible to change the base URL to not include the /v2/nearest prefix, so that it then makes sense to have another API called PriorityNearest that invokes /priority/v2/nearest. 2. whether binding a CLI flag directly to a default value is necessary or whether we can choose a more idiomatic path where the URL is a string, the zero value is valid, we pick the right default base URL if empty, and there is some additional mechanism for setting up the URL from the CLI (this coupling between a library package and the CLI strikes me as "too much coupling" between components). Anyway, for now, this patch should be enough to unblock making progress with m-lab/ndt7-client-go.
Collaborator
Pull Request Test Coverage Report for Build 1639Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This seems the cleanest and less frictious way to enable the m-lab/ndt7-client-go codebase to supply a token for the purpose of identifying the client registration.
Technically speaking, this change is not extending the locate client API for supporting /priority/v2/nearest requests rather it just adds the ability of providing a token. (I do not know who's using this API but I can imagine an organization having their own API requiring a token, though I think it would be useful to clarify what is our user story here.)
As a side effect, m-lab/ndt7-client-go would then choose the right URL and provide a token, still invoking Nearest.
I tried other approaches but the fact that (1) the default base URL includes the /v2/nearest path, (2) the default base URL could be configured using CLI flags and (3) it is not clear to me who are the users of this API prompted me to proceed more cautiously and just make it possible to provide a token.
Ideally, more in the long term, I think it would be useful to confirm the following:
who are the users of m-lab/locate/api/client and whether it is possible to change the base URL to not include the /v2/nearest prefix, so that it then makes sense to have another API called PriorityNearest that invokes /priority/v2/nearest.
whether binding a CLI flag directly to a default value is necessary or whether we can choose a more idiomatic path where the URL is a string, the zero value is valid, we pick the right default base URL if empty, and there is some additional mechanism for setting up the URL from the CLI (this coupling between a library package and the CLI strikes me as "too much coupling" between components).
Anyway, for now, this patch should be enough to unblock making progress with m-lab/ndt7-client-go.
This change is