feat: transfer static asset handling into own package #21785

williamhbaker · 2021-07-02T21:00:50Z

Part of #20832
Closes #21541
Also related to #21652

This PR primarily moves the static UI file handling code out of the chronograf folder, and into it's own separate package. Along with this, I added the necessary build processes to pull in the swagger.json file from the openapi repo and compile that into the binary as well. The swagger file handling and embedding is also part of the new package, which is named static.

The code in static/static.go is almost entirely new, but is intended to duplicate the functionality of the equivalent code in the chronograf and http packages. I had originally written this code to support usage with the go:embed directive, but found it to be not quite appropriate for our use (yet), and adapted it to use the http.FileSystem interface which we can plug the go-bindata abstractions into. This should make it very easy to switch over to using go:embed in the future.

The now dead code in the chronograf package remains mostly untouched. It's hard to delete it just yet because of inter-dependencies with the CLI code, which will hopefully soon be removed as well. In contrast, the now dead code in the http package was completely removed. All static file handling code related to UI assets and the swagger.json is now in the static package.

williamhbaker · 2021-07-02T21:01:36Z

.gitignore

@@ -36,34 +36,15 @@ test.key
 /editorconfig-checker
 /staticcheck

-ui/node_modules


Did some cleanup in here not directly related to this PR since we no longer have these things in the ui directory.

williamhbaker · 2021-07-02T21:02:26Z

chronograf/canned/TODO.go

@@ -1,5 +1,3 @@
-// +build !assets


These 3 build flags are removed so that the placeholders are always in use - otherwise the build would fail because the Makefile doesn't recurse into chronograf anymore.

williamhbaker · 2021-07-02T21:05:34Z

static/Makefile

+
+# Target for the swagger.json from the openapi repo
+data/swagger.json:
+	OPENAPI_SHA=c87a08f832e79bc338a37509028641cda9e1875b ../scripts/fetch-swagger.sh


I don't feel super great about how we are going to keep track of the specific magic strings that link these things together. This one is for the commit in the openapi repo that we want to pull the swagger from. There's not currently one for the master branch for the UI version since it always just pulls the master release tag, but there will be one for release branches.

My thought was that we could make the fetcher scripts take arguments (env vars), and then give them the env vars somehow, which would be specified in a central location. My first guess was within this static package Makefile but there's probably a better way.

I'd typically declare -r OPENAPI_SHA in the fetch-swagger.sh script instead of tracking it here. That'd be ~consistent with how fetch-ui-assets.sh works.

williamhbaker · 2021-07-02T21:08:25Z

static/static_test.go

@@ -0,0 +1,129 @@
+package static


I tried to get as much of the interesting logic as I could easy test covered, but it's not comprehensive. If the handlers don't work right, it's going to be quite obvious from failing e2e tests.

danxmoran · 2021-07-06T14:19:48Z

static/TODO.go

@@ -0,0 +1,28 @@
+// +build !assets


Now that the UI & swagger are getting built externally, do we need to keep this assets flag & the TODO case? It seems to be a frequent tripping point for users trying to build from source by using go build instead of make. Could we instead have the go-build always assume assets are available locally, and raise a build-time error if they're not present?

Based on my attempts, the flag and TODO case are really needed so that the test suite can be run without getting a build error. Things like the golint and gotest CI jobs which run tests without necessarily traversing through & running the go:generate here would fail otherwise since the functions in the TODO file don't exist. I do feel like there might be some way to get around this that I'm missing though.

One of the other benefits of go:embed is that - after a fashion - if the embedding assets aren't generated (like for testing), the tests can still run and the binary can even compile, but we could just send a 404 if one of the assets is requested.

So my position is that: It's quite possible that I'm missing a better way to handle this currently...but if I'm not, I'd punt cleaning up the assets flag until/if we get around to incorporating asset compression and using go:embed after all.

I looked for other ways to work around the ability to run tests without pulling in the external assets and couldn't come up with anything. Packages other than go-bindata also seem to rely on using some kind of go:generate to generate some code that you reference via function or import as a package etc., so it has to exist for tests to be able to run. Build tags seem like the cleanest way to say "under this set of circumstances, use this function...and use a different one under a different set of circumstances". I guess the other option might be to generate all of the assets even for running the unit tests etc., but that seems not ideal for other reasons.

danxmoran · 2021-07-06T14:22:43Z

static/Makefile

+
+# Target for the swagger.json from the openapi repo
+data/swagger.json:
+	OPENAPI_SHA=c87a08f832e79bc338a37509028641cda9e1875b ../scripts/fetch-swagger.sh


I'd typically declare -r OPENAPI_SHA in the fetch-swagger.sh script instead of tracking it here. That'd be ~consistent with how fetch-ui-assets.sh works.

danxmoran · 2021-07-06T14:23:26Z

static/static.go

@@ -0,0 +1,195 @@
+//go:generate env GO111MODULE=on go run github.com/kevinburke/go-bindata/go-bindata -o static_gen.go -ignore 'map|go' -tags assets -pkg static data/...


For a follow-up, I wonder if this module could help us drop go-bindata entirely: https://github.com/vearutop/statigz

Oh cool, that does look nice, especially since it claims to also set etags. I added a note to the go:embed issue for future reference: #21652

williamhbaker added 2 commits July 1, 2021 15:15

feat: transfer static asset handling into own package

1893f02

chore: added some tests

db8a287

williamhbaker commented Jul 2, 2021

View reviewed changes

danxmoran reviewed Jul 6, 2021

View reviewed changes

williamhbaker mentioned this pull request Jul 6, 2021

Use go:embed for serving static UI files #21652

Open

williamhbaker added 2 commits July 6, 2021 10:15

fix: update swagger script per review

a9a1e2f

fix: make into bash script

2273a95

danxmoran approved these changes Jul 6, 2021

View reviewed changes

williamhbaker merged commit ea5e6ea into master Jul 6, 2021

williamhbaker deleted the wb-split-assets-21652 branch July 6, 2021 17:54

danxmoran mentioned this pull request Jul 20, 2021

build: remove as much of chronograf as possible, move static-asset code into static package #21898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transfer static asset handling into own package #21785

feat: transfer static asset handling into own package #21785

williamhbaker commented Jul 2, 2021

williamhbaker Jul 2, 2021

williamhbaker Jul 2, 2021

williamhbaker Jul 2, 2021

danxmoran Jul 6, 2021

williamhbaker Jul 2, 2021

danxmoran Jul 6, 2021

williamhbaker Jul 6, 2021

williamhbaker Jul 6, 2021

danxmoran Jul 6, 2021

danxmoran Jul 6, 2021

williamhbaker Jul 6, 2021

		@@ -0,0 +1,195 @@
		//go:generate env GO111MODULE=on go run github.com/kevinburke/go-bindata/go-bindata -o static_gen.go -ignore 'map\|go' -tags assets -pkg static data/...

feat: transfer static asset handling into own package #21785

feat: transfer static asset handling into own package #21785

Conversation

williamhbaker commented Jul 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment