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
tiltfile: add a force_parallel option for local_resource #3693
Conversation
6ebd8c1
to
c7d859c
Compare
@@ -21,7 +21,7 @@ func NextTargetToBuild(state store.EngineState) *store.ManifestTarget { | |||
|
|||
// Local targets aren't parallelizable with any other kind of target. | |||
// So if we're already building a local target, bail immediately. | |||
if IsBuildingLocalTarget(state) { | |||
if IsBuildingUnparallelizableLocalTarget(state) { |
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.
unless I've misunderstood the intent of this PR, we also need to change the RemoveLocalTargets
bit a few lines down
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.
oooh good catch! fixed and added a test
eb89e22
to
0804ecb
Compare
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.
LGTM!
// Some local targets aren't parallelizable with any other kind of target. | ||
// So if we're already building any targets, remove all the local ones. |
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.
// Some local targets aren't parallelizable with any other kind of target. | |
// So if we're already building any targets, remove all the local ones. | |
// Some local targets aren't parallelizable with any other kind of target. |
just since "all the local ones" is no longer accurate, a better wording doesn't come immediately to mind, and the function name does a pretty good job of communicating this anyway, might as well delete this second line
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.
reworded a bit
Hello @landism,
Please review the following commits I made in branch nicks/ch8725:
12499b4 (2020-08-12 17:45:22 -0400)
tiltfile: add a force_parallel option for local_resource
Code review reminders, by giving a LGTM you attest that: