Skip to content
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

Migrate from go-bindata to vfsgen #1532

Merged
merged 7 commits into from
Sep 24, 2018

Conversation

simonpasquier
Copy link
Member

Closes #1487

Regarding whether to commit the generated files or not, the outcome from prometheus/prometheus#4430 (comment) was to commit so go get ... would work. But this isn't enough for AlertManager since the Javascript code (eg /ui/app/script.js) is also generated but not committed.

@simonpasquier
Copy link
Member Author

Regarding whether to commit the generated files or not

Ok so /ui/app/script.js being generated but not committed, Travis CI fails because the committed asset/assets_vfsdata.go doesn't match with the output of the CI.

@simonpasquier
Copy link
Member Author

To be exhaustive, the CI fails also because just like go-bindata, vfsgen doesn't preserve timestamps (see shurcooL/vfsgen#26). I'm testing some ideas on how we could preserve timestamps since this is hitting us for the other Prometheus projects.

@simonpasquier simonpasquier force-pushed the vfsgen branch 4 times, most recently from e649ca5 to 3a5714a Compare September 6, 2018 13:58
@mxinden
Copy link
Member

mxinden commented Sep 6, 2018

@simonpasquier let us know once this is ready for a review.

@simonpasquier
Copy link
Member Author

Now that the flakes are "fixed", this is ready for review though it deserves a bit of explanation.

To circumvent the problem that the file generation wasn't idempotent, I've implemented the idemvfs package. This package combines a "bare" http.FileSystem object (as generated by vfsgen) with metadata information about the last known modification timestamp + size + SHA1 checksum of each file in the file system. The metadata is generated by asset/modtime_generate.go but most of it is handled in the idemvfsgen package. When vfsgen walks the assets, the file system will return the stored metadata if the size & checksum of the "real" file match otherwise it will return the current information. When make assets is executed, the metadata file is regenerated first then a second command generates the assets. This is a bit convoluted but it works. I still need to write some tests for those packages.

As we have the same issue with idempotency for the other Prometheus projects, it would make sense to put this code in the common repository but maybe it should be tried here for a bit of time?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the profound work here. Especially vendoring all dependencies, only requiring go generate.

I am still a bit scared of the introduced complexity to enforce idempotency for timestamps, with previously simply setting all timestamps to 1.

Running this locally works fine except this small formatting issue:

diff --git a/asset/assets_vfsdata.go b/asset/assets_vfsdata.go
index ad00a03..fc6f1cf 100644
--- a/asset/assets_vfsdata.go
+++ b/asset/assets_vfsdata.go
@@ -214,7 +214,7 @@ func (fs vfsgen۰FS) Open(path string) (http.File, error) {
                }
                return &vfsgen۰CompressedFile{
                        vfsgen۰CompressedFileInfo: f,
-                       gr: gr,
+                       gr:                        gr,
                }, nil
        case *vfsgen۰FileInfo:
                return &vfsgen۰File{

But that just might be my setup (CI runs green) with Go 1.11.

@@ -0,0 +1,163 @@
// Code generated by idemvfsgen; DO NOT EDIT.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the metadata file you referenced in https://github.com/prometheus/alertmanager/pull/1532/#issuecomment-420321115 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@stuartnelson3
Copy link
Contributor

But that just might be my setup (CI runs green) with Go 1.11.

I've encountered this in a couple PRs -- gobindata is formatted differently between 1.10 and 1.11 :/

Signed-off-by: Simon Pasquier <[email protected]>
This is identical to what we had with go-bindata and avoids the extra
step of storing the identity of the complete file system in another
location.

Signed-off-by: Simon Pasquier <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>
@simonpasquier
Copy link
Member Author

@mxinden @stuartnelson3 for simplicity, I've implemented something similar to what we had with go-bindata so all files/directories have a modification time of time.Unix(1, 0). This is achieved by having a simple wrapper around http.FileSystem (see https://github.com/prometheus/alertmanager/pull/1532/files#diff-b08beeb7b47ec7a1eba03e3226528c33). As discussed directly with Max, this is good enough for now and provides exactly the same functionality as before.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the simplification. If there are no further comments by @stuartnelson3 I will merge tomorrow.

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants