-
Notifications
You must be signed in to change notification settings - Fork 9
Bug 1428422 - add ROOTURL to Credentials struct #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments here, for consistency with other clients.
codegenerator/model/api.go
Outdated
BaseURL: DefaultBaseURL, | ||
Authenticate: credentials != nil, | ||
Service: "` + api.ServiceName + `", | ||
Version: "` + "v1" + `", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhford has added this to the API reference as api.apiVersion
(defaulting to "v1"
if not present)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great, the "v1"
was a stop gap until we had somewhere to get this data from, I'll update it!
codegenerator/model/api.go
Outdated
// TASKCLUSTER_ROOT_URL | ||
// | ||
// If environment variable TASKCLUSTER_ROOT_URL is empty string or not set, | ||
// https://taskcluster.net will be assumed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have not set a default value for rootUrl in the JS client. I worry that this is going to hide places where we missed setting rootUrl, letting them appear when we shut down taskcluster.net
. Admittedly, there are fewer of those to worry about than for JS or Python, since basically only generic-worker uses the go client. Still, I'd like such errors to surface as quickly and easily as possible. For example, that made porting services to the new version of the JS client much easier, by just running the tests until they stopped failing with "rootUrl must be specified" :)
That new requirement is the reason for the major version change on the JS and Python libraries. I don't know if it's worth doing the same for this library (it requires a v2/
directory to make a breaking change in Go, right?), or given the limited usage of the library we can slide the change in in the same repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is funny, I had it in mind to go back and change this, it looks like I must have forgotten to - but I came to the same reasoning, so thanks for reminding me! Indeed, we also don't want external organisations to run their client, forget to export TASKCLUSTER_ROOT_URL
in their environment (for example) and then suddenly (and surprisingly) connect to our production cluster. In the worst case they might call an unauthenticated endpoint (such as listing clients) and actually get back real data, rather than fail out with an error. That indeed would be undesirable! So, I agree 100%, and am also grateful you took the time to look through the code to see what it was doing. :-)
creds.go
Outdated
// Version of client API, e.g. "v1" | ||
Version string | ||
// Service name, e.g. "aws-provisioner" | ||
Service string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we call this ServiceName? To be consistent with the terminology used elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
creds.go
Outdated
type Client struct { | ||
// Version of client API, e.g. "v1" | ||
Version string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ApiVersion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll call it APIVersion
so golint
doesn't complain. Good idea!
@petemoore we accomplished this a different way eventually, right? Should this be closed? |
Work in progress...