-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Replace go-bindata-assetfs build dependency with native go:embed #11208
Conversation
Awesome! The bootstrap process can probably be simplified too (https://github.com/hashicorp/vault/blob/master/tools/tools.go#L25-L29) |
Indeed, I did that in cff9f7a shortly after you mentioned it. Thanks! |
I think there are a few more references in the Makefile to remove: Lines 11 to 12 in 1b96d2c
|
move web_ui to http remove web ui files, add .gitkeep updates, messing with gitkeep and ignoring web_ui update ui scripts gitkeep ignore http/web_ui Remove debugging remove the jwt reference, that was from something else restore old jwt plugin move things around Revert "move things around" This reverts commit 2a35121. Update ui path handling to not need the web_ui name part add desc move the http.FS conversion internal to assetFS update gitignore remove bindata dep clean up some comments remove asset check script that's no longer needed Update readme remove more bindata things restore asset check update packagespec update stub stub the assetFS method and set uiBuiltIn to false for non-ui builds update packagespec to build ui
* master: (78 commits) docs: update vault-helm to 0.11.0 (#11355) Add documentation for vault-csi-provider namespace config (#11344) docs: update vault-k8s to 0.10.0 (#11354) patch(docs): fix link color (#11352) Add TFE/TFC auth plugin to plugin portal (#11348) fix a couple typos (#11343) TLS Diagnose Formatting Fixes (#11342) Add More TLS Tests and Verification of TLS Root Certificate (#11300) Add HA only autopilot to changelog (#11339) Support autopilot when raft is for HA only (#11260) Fixes for db connection file type field (#11331) Fix flakey TestAgent_Template_Retry test (#11332) Darwin/ARM64 build target (#11321) Fix broken OIDC Providers link (#11327) Bug: DB secret engine not showing "Select one" in role select options (#11294) bumping alpine version, improving security (#11271) Run a more strict formatter over the code (#11312) docs: add persistent cache (#11272) Fix a few static analysis findings (#11307) Changing from "changelog" to "release-note" (#11303) ...
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.
Looks good! I tested it locally, and it works well! We may want to try building with package spec too!
Hey @ncabatoff -
Some background: the previous bindata-assetFS approach converted the assets into
(from https://golang.org/pkg/embed/#hdr-Directives) I found that I couldn't reference a directory above the directory I was in, so I couldn't do this: //go:embed ../pkg/web_ui/*
var content embed.FS As a result, I had to move things into the We could alternatively start to track them if we want, since now they'll be included in the binaries with the UI instead of converted by bindata-assetFS. What do you think? |
cc: @chelshaw @Monkeychip for awareness |
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.
Just one comment, but looks good otherwise!
This actually breaks vendoring the vault as a go module since those ui files are not part of the repo. $ go get -d github.com/hashicorp/vault@main
go: module github.com/golang/protobuf is deprecated: Use the "google.golang.org/protobuf" module instead.
$ go mod vendor
go mod vendor: pattern web_ui/*: no matching files found |
@heppu We don’t really support vendoring Vault as a go module, as that can often run into conflicts when importing both Vault, along with the |
Yes I have encountered those issues previously but then again those are issues that user can always resolve with replace rules and some detective work on github. This how ever is something that goes straight against the Go's tooling and hence there is no nice way around. I guess what I'm asking here is that is there something preventing us from adding those web ui files in the repository? |
@heppu if you'd like to raise a PR fixing this issue, please feel free to do so, and we'll be happy to review it. I'm not sure if there's something preventing us from including the web_ui files, I think that was a decision we'd be ok revisiting if there's a PR making a case for that to be changed. |
@pmmukh Thanks for quick responses, I will do that! |
This PR replaces Vault's use of elazarl/go-bindata-assetfs in building the UI with Go's native Embed package.