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.

Sign up for GitHub

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

Jump to bottom

Support parallel local builds (defaults to sequential) #3471

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Support parallel local builds (defaults to sequential) #3471

merged 1 commit into from
Jan 8, 2020

Conversation

dgageot
Copy link
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

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
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.

pkg/skaffold/schema/defaults/defaults.go Outdated Show resolved Hide resolved
@loosebazooka
Copy link
Member

Is it possible to selectively disable parallel builds?

@dgageot
Copy link
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
Member

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

@dgageot
Copy link
Contributor Author

dgageot commented Jan 7, 2020

@loosebazooka not for now

nkubala
nkubala reviewed Jan 7, 2020
View reviewed changes
Copy link
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

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
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
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
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
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
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

@nkubala nkubala nkubala approved these changes

@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

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