-
Notifications
You must be signed in to change notification settings - Fork 274
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
feat(container): experimental cloudbuilder support #5928
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.
Great stuff! Just commenting for how since this is still marked as a draft.
Experimental cloudbuilder support for test customers.
1eb78e8
to
1efc066
Compare
6895963
to
f93fdb4
Compare
32d8371
to
083fc92
Compare
…ly using / downloading the tool
083fc92
to
8420f1d
Compare
proc.on("exit", () => { | ||
resolve() | ||
}) |
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.
Should we simplify this to be just proc.on("exit", resolve)
like in the line above?
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.
We can do that, but that'd change the behaviour; Right now the Promise
resolves to void
and in your variant it'd resolve to whatever the exit
event handler receives as first argument (Likely the exit code). I only added the error
here as I noticed that in case of an error the Promise would never be resolved or rejected. The error
event will pass the error itself to reject
as first argument.
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, right! Thanks for the explanation :)
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.
Thank you! 💯 LGTM! 👍
I left some comments, please ping me when it's ready for the re-review :)
|
||
let isCloudBuilderEnabled = containerProvider.config.gardenCloudBuilder?.enabled || false | ||
|
||
// The env variable GARDEN_CLOUD_BUILDER can be used to override the cloudbuilder.enabled config setting. |
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.
The setting name seems to be gardenCloudBuilder.enabled
.
}) | ||
|
||
// public API | ||
export const cloudbuilder = { |
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.
Let's rename this to cloudBuilder
.
Thank you! 👍 The comments above are not critical and can be addressed in a follow-up PR. |
What this PR does / why we need it:
Experimental cloudbuilder support for alpha preview users.
Which issue(s) this PR fixes:
Implements Garden Cloud Builder support, but also Fixes #5437
Special notes for your reviewer:
Tests are missing. I'd like to start dogfooding and validating this, and then implement tests before we make it generally available.