-
Notifications
You must be signed in to change notification settings - Fork 136
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
allowing on cluster build for go runtime #1445
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ func validatePipeline(f fn.Function) error { | |
return ErrRuntimeRequired | ||
} | ||
|
||
if f.Runtime == "go" || f.Runtime == "rust" { | ||
if f.Runtime == "rust" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could remove this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be better to keep it here, to explicitly tell user about not supported runtime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the issue with Go and Rust is that they use custom buildpacks. What this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zroubalik @Shashankft9 what about just printing warning here "Go/Rust default builder do not support in cluster build" instead of returning an error? Could we accept that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Shashankft9 Can you add a simple warning here that Go and Rust default builders do not support in-cluster builds? |
||
return ErrRuntimeNotSupported{f.Runtime} | ||
} | ||
|
||
|
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.
Builder should be
"pack"
.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.
but empty string probably would work 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.
If these two items are intended as hints, commenting them would probably be best.
The idea here is that the code has static defaults. Anything placed in the config file is specifically requesting that exact value, so while the first value,
[]buildpacks
is innocuous, the second,builder=""
is, if literally translated, requesting the system to build without a builder. In this case we do set the default if the value comes in empty like this, because building without a builder is nonsensical, but I presume this exists to give a hint to the user how to choose a builder and buildpacks, so a commented entry is the right tool for the job.