-
Notifications
You must be signed in to change notification settings - Fork 2
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
Save and Publish Annotations #6
Conversation
… to being published to UPP. Refactoring resource tests.
…s to api.yml. Making build work
annotations/writer.go
Outdated
defer resp.Body.Close() | ||
|
||
if resp.StatusCode != http.StatusOK { | ||
return nil, fmt.Errorf("Save to %v returned a %v status code", p.saveEndpoint, resp.StatusCode) |
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.
Would it be useful to return the concrete URL in the message rather than the template (p.saveEndpoint
) here ?
type Writer interface { | ||
Write(uuid string, tid string, body map[string]interface{}) (map[string]interface{}, error) | ||
GTG() error | ||
Endpoint() 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.
Is it really useful to have Endpoint()
as an exported method here? We could have GTG()
return (string,error)
instead - I don't think we're implementing an interface here - which would allow us to encapsulate control of the error message.
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 did the same thing for the Publisher interface so kept it consistent - its sole purpose currently is to return a meaningful message during a healthcheck, so yes we could change the interface to return the endpoint and the gtg error
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.
Actually it's not trivial to split these up, as the Endpoint()
call is used directly in the healthcheck setup, whereas the GTG
is called on invocation - is it fine to leave as is?
@@ -74,16 +74,31 @@ func main() { | |||
EnvVar: "API_YML", | |||
}) | |||
|
|||
saveEndpoint := app.String(cli.StringOpt{ | |||
Name: "annotations-save-endpoint", |
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 this have a useful default value?
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.
What would you suggest the default should be? It would need to be either a local or test draft-annotations-api
endpoint
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 seems to me that draft-annotations-api
world work in k8s everywhere. (Having a default value also helps to clarify whether it should have a protocol on the front or not.)
|
||
saveGTGEndpoint := app.String(cli.StringOpt{ | ||
Name: "annotations-save-gtg-endpoint", | ||
Desc: "GTG Endpoint for the service which saves PAC annotations (usually draft-annotations-api)", |
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.
Same comment as above.
…l in case of PUT failure.
Annotations are now saved to the configured save endpoint prior to being published to UPP. Refactoring resource tests.