Skip to content
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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dTechLife
Copy link

@dTechLife dTechLife commented Nov 17, 2021

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:

  • Discuss with the caprover team I haven't spoken on the slack group yet.
  • Update the docs
  • Test functionality
  • Make sure I'm not breaking anything since this is my first commit to the project and I'm not 100% familiar with the codebase.

@dTechLife dTechLife changed the title Feature add build override options FR: add build override options Nov 17, 2021
Copy link
Collaborator

@githubsaturn githubsaturn left a 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
Copy link
Collaborator

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

Copy link
Author

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,
Copy link
Collaborator

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?

Copy link
Author

@dTechLife dTechLife Nov 18, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants