Version Endpoint #2009
Version Endpoint #2009
Conversation
| }{ | ||
| Revision: revision, | ||
| Version: version, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Updated the version endpoint to create a static response once at startup, mimicking the pattern used for the bidder info handler. Changed response structure to list the revision first to match PBS-Java. This is not a breaking change for the admin endpoint and we might as well make it match for the public debut.
| @@ -310,6 +311,7 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R | |||
| r.POST("/cookie_sync", endpoints.NewCookieSyncEndpoint(syncersByBidder, cfg, gdprPerms, r.MetricsEngine, pbsAnalytics, activeBidders).Handle) | |||
| r.GET("/status", endpoints.NewStatusEndpoint(cfg.StatusResponse)) | |||
| r.GET("/", serveIndex) | |||
| r.Handler("GET", "/version", endpoints.NewVersionEndpoint(version.Ver, version.Rev)) | |||
There was a problem hiding this comment.
The endpoint registration looks different because it's adapting to the http.Handle interface already in use by the admin endpoint. The r.GET call is just syntactical sugar around this anyway.
endpoints/version.go
Outdated
| } | ||
| if revision == "" { | ||
| revision = versionNotSet |
There was a problem hiding this comment.
Nitpick: This could be a bit confusing. Ideally we should assign revisionNotSet and not versionNotSet to revision. Do you think renaming versionNotSet to either notSet or valueNotSet could be more straightforward?
There was a problem hiding this comment.
Renamed for clarity. I cannot rename it notSet or valueNotSet because it's in the same package (naming scope) as other endpoints.
| }) | ||
| if err != nil { | ||
| glog.Errorf("/version Critical error when trying to marshal versionModel: %v", err) | ||
| w.WriteHeader(http.StatusInternalServerError) |
There was a problem hiding this comment.
I see we are not using w.WriteHeader(http.StatusInternalServerError) in the new version. Why are we not writing this error anymore?
There was a problem hiding this comment.
I moved the json marshal outside of the endpoint, so that it happens only once at application startup. If it fails, PBS will panic. Thus, it's no longer possible for the error to occur within the endpoint.
| version: "1.2.3", | ||
| revision: "d6cd1e2bd19e03a81132a23b2025920577f84e37", | ||
| expected: `{"revision":"d6cd1e2bd19e03a81132a23b2025920577f84e37","version":"1.2.3"}`, | ||
| }, |
There was a problem hiding this comment.
Can we include these additional two tests?
{
description: "Empty",
version: "1.2.3",
revision: "",
expected: `{"revision":"not-set","version":"1.2.3"}`,
},
{
description: "Populated",
version: "",
revision: "d6cd1e2bd19e03a81132a23b2025920577f84e37",
expected: `{"revision":"d6cd1e2bd19e03a81132a23b2025920577f84e37","version":"not-set"}`,
},
Implements #1172