Skip to content

tiltfile/engine: localresource execs in correct dir [ch3505]#2251

Merged
maiamcc merged 4 commits into
masterfrom
maiamcc/workdir-local-resource
Sep 24, 2019
Merged

tiltfile/engine: localresource execs in correct dir [ch3505]#2251
maiamcc merged 4 commits into
masterfrom
maiamcc/workdir-local-resource

Conversation

@maiamcc

@maiamcc maiamcc commented Sep 24, 2019

Copy link
Copy Markdown
Contributor

Hello @jazzdan, @dbentley,

Please review the following commits I made in branch maiamcc/workdir-local-resource:

158ba6c (2019-09-24 12:24:55 -0400)
run cmd from wd, test

77da300 (2019-09-24 11:58:58 -0400)
attach workdir to LocalTarget

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
maiamcc requested review from dbentley and jazzdan September 24, 2019 16:29

ps := build.NewPipelineState(ctx, 1, bd.clock)
ps.StartPipelineStep(ctx, "Running command: %v", c.Argv)
ps.StartPipelineStep(ctx, "Running command: %v (from %q)", c.Argv, wd)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
ps.StartPipelineStep(ctx, "Running command: %v (from %q)", c.Argv, wd)
ps.StartPipelineStep(ctx, "Running command: %v (in %q)", c.Argv, wd)

cmd := exec.CommandContext(ctx, c.Argv[0], c.Argv[1:]...)
cmd.Stdout = writer
cmd.Stderr = writer
if wd != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this does anything. If Dir is empty the command runs relative to the working directory, which is what the default behavior is anyways, since the empty value for a string in a struct is "".

@maiamcc
maiamcc merged commit 6e6dd14 into master Sep 24, 2019
@maiamcc
maiamcc deleted the maiamcc/workdir-local-resource branch September 24, 2019 19:12
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.

2 participants