-
Notifications
You must be signed in to change notification settings - Fork 858
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
FR: add build override options #1264
base: master
Are you sure you want to change the base?
FR: add build override options #1264
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.
Thanks for the PR.
Overall it looks good. Just a few comments. Also, please test this to make sure it's working as expected.
@@ -243,7 243,8 @@ class DockerApi { | |||
tarballFilePath: string, | |||
buildLogs: BuildLog, | |||
envVars: IAppEnvVar[], | |||
registryConfig: DockerRegistryConfig | |||
registryConfig: DockerRegistryConfig, | |||
overrideOptions: string[] | undefined |
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.
Must be renamed to dockerBuildOverrideOptions
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.
Thanks, will do.
@@ -268,6 269,7 @@ class DockerApi { | |||
const optionsForBuild: Dockerode.ImageBuildOptions = { | |||
t: imageName, | |||
buildargs: buildargs, | |||
...overrideOptions, |
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.
Is this how it's passed to Dockerode? Isn't it using a key<>value pair structure?
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 way to word that question; it isn't! I'll take a look at Dockerode and make sure I'm passing the variable correctly. :) Thank you for taking the time to review this for me!
I'll go through the concerns in the review, commit, build and test the functionality of the feature and change this from a draft after I confirm it works!
3e708d7
to
46e7e20
Compare
This isn't complete but I wanted to make sure I am going down the right path.
I brought up in an old issue, I'd like to know more about the implications of making this change.
I believe caprover uses /var/run/docker.sock to spawn build containers, with this change to the server a user should be able to add extra build option flags to be passed to the docker build command.
https://github.com/caprover/caprover/blob/master/CONTRIBUTING.md
TODO: