Skip to content

Support parallel local builds (defaults to sequential)#3471

Merged
nkubala merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:local-parallel
Jan 8, 2020
Merged

Support parallel local builds (defaults to sequential)#3471
nkubala merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:local-parallel

Conversation

@dgageot
Copy link
Copy Markdown
Contributor

@dgageot dgageot commented Jan 2, 2020 •
edited by briandealwis
Loading

Had to cherry pick the schema change from #3453

Fixes #1380

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 2, 2020 •
edited
Loading

Codecov Report

Merging #3471 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/build/local/local.go 54.54% <100%> (ø) ⬆️
pkg/skaffold/schema/defaults/defaults.go 94.31% <100%> (+0.34%) ⬆️
pkg/skaffold/build/local/types.go 68.57% <100%> (ø) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0%> (+2.43%) ⬆️

@dgageot dgageot force-pushed the local-parallel branch 2 times, most recently from b803822 to 08d07cf Compare January 3, 2020 15:09
@dgageot dgageot added size/M and removed size/XXL labels Jan 6, 2020
briandealwis
briandealwis reviewed Jan 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my concern here is that concurrent builds are enabled for all local builders. But Jib-based multi-module projects (artifacts that share the same context / workspace) will encounter problems.

@loosebazooka
Copy link
Copy Markdown
Member

loosebazooka commented Jan 7, 2020

Is it possible to selectively disable parallel builds?

@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jan 7, 2020

@loosebazooka selectively how? With this PR, local parallel builds are disabled by default. Users can activate them by changing the skaffold.yaml.

@loosebazooka
Copy link
Copy Markdown
Member

loosebazooka commented Jan 7, 2020

I see, can each builder decide if it wants to participate in parallel builds?

@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jan 7, 2020

@loosebazooka not for now

nkubala
nkubala reviewed Jan 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this is great. would you mind documenting this in https://github.com/GoogleContainerTools/skaffold/blob/master/docs/content/en/docs/pipeline-stages/builders/_index.md?

@dgageot dgageot added the docs-modifications runs the docs preview service on the given PR label Jan 8, 2020
@container-tools-bot
Copy link
Copy Markdown

container-tools-bot commented Jan 8, 2020

Please visit http://35.236.51.86:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Jan 8, 2020
@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jan 8, 2020

@nkubala This is documented on that page. See http://35.236.51.86:1313/docs/pipeline-stages/builders/

@dgageot
Support // builds with local builder
c9a0a87 
Defaults to a sequential build

Fixes GoogleContainerTools#1380

Signed-off-by: David Gageot <david@gageot.net>
@nkubala
Copy link
Copy Markdown
Contributor

nkubala commented Jan 8, 2020

@dgageot well, the concurrency flag is documented, but there isn't really anything saying "we support building locally in parallel, but it's disabled by default". it would be nice to say something like that so people know the feature exists

nkubala
nkubala approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll approve this, but i'd like to see the docs on building improved to discuss sequence vs. parallel and how this can be configured by users. right now it's probably hard for users to discover this

@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jan 8, 2020

Let’s do that. Can you merge this one and I’ll edit the doc tomorrow for both local and remote builders?

@nkubala nkubala merged commit 45c887d into GoogleContainerTools:master Jan 8, 2020
@nkubala
Copy link
Copy Markdown
Contributor

nkubala commented Jan 8, 2020

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@briandealwis briandealwis briandealwis left review comments

@balopat balopat Awaiting requested review from balopat

@chanseokoh chanseokoh Awaiting requested review from chanseokoh

@loosebazooka loosebazooka Awaiting requested review from loosebazooka

@TadCordle TadCordle Awaiting requested review from TadCordle

@tejal29 tejal29 Awaiting requested review from tejal29

+1 more reviewer

@nkubala nkubala nkubala approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add parallel local image building.

6 participants

@dgageot @loosebazooka @container-tools-bot @nkubala @briandealwis @googlebot