Installer API: Add buildTime and tectonicVersion by trawler · Pull Request #2915 · coreos/tectonic-installer

trawler · 2018-02-08T10:06:00Z

this will add a new endpoint /releases/current to the API
which will fetch TECTONIC_VERSION from the build environment
and displays the build time in a JSON format.

coreosbot · 2018-02-08T10:06:02Z

Can one of the admins verify this patch?

cpanato · 2018-02-08T10:17:21Z

installer/api/api.go


 	//handlers_latest_release.go
 	mux.Handle("/releases/latest", logRequests(httpHandler("GET", ctx, latestReleaseHandler)))
+	mux.Handle("/releases/build", logRequests(httpHandler("GET", ctx, currentReleaseHandler)))


need to use the name build? because the other is using latest and this new endpoint is to get the current. then maybe use current instead of build

I was deliberating between the two (I actually created the endpoint with "current" to start with). I thought that "current" sounded weird. can be changed, if you think it's better.

trawler · 2018-02-08T10:19:34Z

Output example:

trawler · 2018-02-08T10:23:50Z

changed endpoint from /releases/build to /releases/current per @cpanato suggestion.

alexsomesan

Very cool solution, Karen! 👍

squat

Overall looks like a fine approach. Needs a little bit of clean up

squat · 2018-02-08T10:37:23Z

installer/stamp/stamp.go

@@ -0,0 +1,7 @@
+package stamp


What does stamp mean? Is it timestamp? It is convention to call this package version, see for example https://github.com/coreos/matchbox/blob/master/matchbox/version/version.go or https://github.com/coreos-inc/bridge/blob/master/version/version.go. Please rename for legibility and consistency

The name was taken from a bazel example of using x_defs to add variables to compiled binaries, and they named it "stamped bin". But your suggestion is actually clearer.

squat · 2018-02-08T10:39:23Z

installer/stamp/stamp.go

+package stamp
+
+// TectonicVersion - defaults to 'dev-build' if TECTONIC_VERSION was not set
+var TectonicVersion = "dev-build"


For simplicity can we just call this Version?

squat · 2018-02-08T10:43:42Z

installer/api/handlers_latest_release.go

+func currentReleaseHandler(w http.ResponseWriter, req *http.Request, _ *Context) error {
+
+	verMap := map[string]string{
+		"TectonicVersion": stamp.TectonicVersion,


This returns a json object with title-cased keys, which is a little odd. By convention, json keys are normally camel-cased, or sometimes snake-cased. Can you fix the casing?

squat · 2018-02-08T10:46:47Z

installer/api/handlers_latest_release.go

+// Fetch tectonic's Build version and return in JSON format
+func currentReleaseHandler(w http.ResponseWriter, req *http.Request, _ *Context) error {
+
+	verMap := map[string]string{


For cases such as this where all the fields are known, we typically use an anonymous struct rather than a map. This should also be more memory-efficient

squat · 2018-02-08T10:47:58Z

installer/api/handlers_latest_release.go

+// Fetch tectonic's Build version and return in JSON format
+func currentReleaseHandler(w http.ResponseWriter, req *http.Request, _ *Context) error {
+
+	verMap := map[string]string{


There is no need to specify the type of this variable in the name. Please rename to just “version”

squat · 2018-02-08T10:50:13Z

installer/api/handlers_latest_release.go

+		"TectonicVersion": stamp.TectonicVersion,
+		"BuildTime":       stamp.BuildTime,
+	}
+	return writeJSONResponse(w, req, http.StatusOK, verMap)


Where is this API endpoint consumed in the front end? Without it, this pull request does not fully solve our issue of the reported build version being wrong

I'm planning to add that change in a separate commit

Sounds good. Separate commit in this same PR or a different PR?

squat · 2018-02-08T10:54:42Z

buildvars.sh

+#!/bin/bash
+
+# Vars exported to the build info
+echo TECTONIC_VERSION ${TECTONIC_VERSION}


Who sets this environment variable? Please add this detail to the build documentation. For consistency, with the rest of the build process, I think that this variable should be simply VERSION

I'd actually prefer to keep these environment inputs namespaced like this, with TECTONIC_*

Two reasons for this:

VERSION is much too common and the probability of it affecting other tools or just being used for something else in the system seems rather high.

The prefixing helps to visualize the current state of the environment much easier by piping env to sort.

that's fine; the main issues here are twofold:

Who sets this variable? We need this documented somewhere.

We need to make this consistent with the rest of the build process; if we want to go the namespaced route, then that means changing the variables used by the tarball bazel build rule so that we do not need to set two different variables.

I agree with @alexsomesan in that using just VERSION would be too generic. We could use this api endpoint in the future, to output any kind of build information that would help us debug issues, such as go / bazel versions, etc, etc. So I'd opt for modifying the documentation and existing vars.

Sounds good. Please make those changes in this PR then

@squat Both are valid questions.
I think for 1) it's whatever RA ends up running in - most likely their Jenkins pipeline.

For 2), if there're no hard blockers, I'd prefer we align everything that's an input to the build process under TECTONIC_ or TECTONIC_BUILD_. We have to keep in mind that the CLI will also support env var overrides. We might want to make a distinction between the runtime and the buildtime inputs (namespaces). But that's mostly off-topic for this change.

squat · 2018-02-08T10:55:01Z

buildvars.sh

@@ -0,0 +1,5 @@
+#!/bin/bash


please add -e here

kyoto · 2018-02-09T01:48:46Z

@kalmog Looks like the commit comment needs updating with the new API endpoint and JSON keys.

Let me know if you need any help with the frontend changes.

squat · 2018-02-12T14:35:03Z

installer/api/handlers_latest_release.go

+
+// Fetch tectonic's Build version and return in JSON format
+func currentReleaseHandler(w http.ResponseWriter, req *http.Request, _ *Context) error {
+


nit: can you eliminate this extra newline?

squat

I have one small nit, otherwise looks good!

trawler · 2018-02-12T15:12:02Z

ok to retest

trawler · 2018-02-12T15:13:32Z

retest this please

this will add a new endpoint "/releases/current" to the API which will fetch TECTONIC_VERSION from the build environment and displays the build time in a JSON format.

trawler requested review from alexsomesan and squat February 8, 2018 10:06

trawler added the tectonic/frontend label Feb 8, 2018

cpanato added the run-gui-tests label Feb 8, 2018

trawler removed the tectonic/frontend label Feb 8, 2018

cpanato added the tectonic/frontend label Feb 8, 2018

cpanato reviewed Feb 8, 2018

View reviewed changes

trawler force-pushed the master branch from 51d192d to d7c1ec8 Compare February 8, 2018 10:22

alexsomesan previously approved these changes Feb 8, 2018

View reviewed changes

squat suggested changes Feb 8, 2018

View reviewed changes

trawler dismissed alexsomesan’s stale review via b1351a0 February 8, 2018 18:44

trawler force-pushed the master branch from d7c1ec8 to b1351a0 Compare February 8, 2018 18:44

squat reviewed Feb 12, 2018

View reviewed changes

trawler force-pushed the master branch from b1351a0 to 383e14f Compare February 12, 2018 14:37

squat previously approved these changes Feb 12, 2018

View reviewed changes

trawler dismissed squat’s stale review via ae8e25b February 12, 2018 15:44

trawler force-pushed the master branch 6 times, most recently from a10e771 to a01ab31 Compare February 12, 2018 15:53

Installer API: Add buildTime and tectonicVersion

a80b00e

this will add a new endpoint "/releases/current" to the API which will fetch TECTONIC_VERSION from the build environment and displays the build time in a JSON format.

trawler force-pushed the master branch from a01ab31 to a80b00e Compare February 12, 2018 16:06

squat approved these changes Feb 12, 2018

View reviewed changes

trawler changed the title ~~Installer API: Add BuildTime and TectonicVersion~~ Installer API: Add buildTime and tectonicVersion Feb 12, 2018

trawler merged commit 311b0e6 into coreos:master Feb 12, 2018


		// Fetch tectonic's Build version and return in JSON format
		func currentReleaseHandler(w http.ResponseWriter, req http.Request, _ Context) error {

Conversation

trawler commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreosbot commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trawler commented Feb 8, 2018

Uh oh!

trawler commented Feb 8, 2018

Uh oh!

alexsomesan left a comment

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trawler Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyoto commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

trawler commented Feb 12, 2018

Uh oh!

trawler commented Feb 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

trawler commented Feb 8, 2018 •

edited

Loading

trawler Feb 8, 2018 •

edited

Loading