-
Notifications
You must be signed in to change notification settings - Fork 183
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
[devbox] Allow for custom install/build/start commands #43
Conversation
This addresses #2 |
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.
LGTM
@@ -6,21 +6,21 @@ package planner | |||
type Planner interface { | |||
Name() string | |||
IsRelevant(srcDir string) bool | |||
Plan(srcDir string) *BuildPlan | |||
GetPlan(srcDir string) *Plan |
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.
Why the name change? Plan fits nicely with the Planner interface name and is a little more idiomatic.
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.
I wanted to use Plan
for the struct (since BuildPlan
feels like a misnomer when we also use it to start the shell, and then go
complained that the struct couldn't be called Plan
at the same time as the method)
Thoughts?
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, I see. I'd just go ahead and merge then. We can think about the names later.
planner/go_planner.go
Outdated
InstallCommand: "go get", | ||
BuildCommand: "CGO_ENABLED=0 go build -o out", | ||
StartCommand: "./out", // TODO: Move gin specific stuff elsewhere. | ||
InstallStep: &Step{ |
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.
For packages, will mergo append go
on top of the packages
definition in devbox.json
config?
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.
If so, should we delete the go
package in go_planner
?
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.
Not sure I followed everything.
- Packages will get appended and duplicates removed (see planner/plan_test.go)
- We need
go
in order to callgo get
even in the case the user has not requested that package.
Is that what you were asking?
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.
So if a user added go_18-x
, would both go_18-x
and go
end up being installed in the container?
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.
That would cause an error (see #33), but I believe the fix is to improve the planner code to detect the version of go (which is stated in the go.mod
), and add the right package
Summary
Allow for custom install/build/start commands by customizing them in
devbox.json
. This makes it possible for users to build docker images in the cases when they need to do some customization, or when we have not yet written a language planner for that language.How was it tested?
Built, and ran
devbox plan
anddevbox generate
against a few examples.