- Notifications
You must be signed in to change notification settings - Fork928
feat: remove server subcommand from slim binaries#5747
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Looking good, had some small suggestions but pre-approving.
buildinfo/buildinfo.go Outdated
| @@ -73,6 +75,16 @@ func IsDev() bool { | |||
| return strings.HasPrefix(Version(), develPrefix) | |||
| } | |||
| // IsSlim returns true if this is a slim build. | |||
| func IsSlim() bool { | |||
| return strings.Contains(slim, "t") | |||
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.
Could we set this via build tag so that it's present when doinggo build as well? (I.e. we don't need to inject it via ldflags.)
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.
Like by usinggo build -tags slim?
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.
Yup
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.
Sure, but we won't be able to detect agpl this way since we don't use a flag for it.
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.
Done for slim, AGPL still uses ldflags. I've added a comment tobuild_go.sh why we don't provide anagpl build flag
Uh oh!
There was an error while loading.Please reload this page.
| "github.com/spf13/viper" | ||
| "github.com/coder/coder/cli/deployment" | ||
| "github.com/coder/coder/coderd" |
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.
Importingcoderd here is unfortunate. I think we could reduce binary size if we didn't but I might be wrong. I think we can avoid it if we change enterprise slim too?
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.
Sadly alsoroot.go uses this function so the signature needs to match on both. I think this could be done as a second improvement, but I want to get this in so we can unblock the community PR#5738
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.
Ah, didn't think of that. Yes makes sense 👍🏻.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
slimtag if this is a slim buildIsSlim()andIsAGPL()tobuildinfo, and add those details tocoder versionserver_slim.go(in both AGPL and enterprise CLI) which only builds withslimtagserveHandlerfromserver.gotoagent.goThis doesn't fully remove the server code from slim binaries, as the packages are still imported, but it disables them.