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

Report signalled commands #906

Merged
merged 2 commits into from May 15, 2019

Conversation

Projects
None yet
3 participants
@DazWorrall
Copy link
Member

commented May 14, 2019

Closes #882

The purpose of this change is to have exit_message show something meaningful when a command is signalled to exit.

end

def code
@status&.exitstatus

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall May 14, 2019

Author Member

The & guard is to maintain the same API that the previous attr_reader provided.

sleep 0.05
retry
end
if reap_child!(block: false)

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall May 14, 2019

Author Member

This had no tests previously and is very difficult to test for - trying to reap a process not yet ready to exit. I've eyeballed this carefully and expect that I've preserved the existing behaviour, please check my working here.

Show resolved Hide resolved lib/shipit/command.rb

@DazWorrall DazWorrall requested review from wvanbergen and Shopify/pipeline May 14, 2019

@wvanbergen
Copy link
Member

left a comment

Couple of remarks.

Show resolved Hide resolved lib/shipit/command.rb
Show resolved Hide resolved lib/shipit/command.rb
Show resolved Hide resolved lib/shipit/command.rb
Show resolved Hide resolved lib/shipit/command.rb
command = Command.new('sleep 10', chdir: '.')
t = Thread.new do
signalled = false
20.times do

This comment has been minimized.

Copy link
@wvanbergen

wvanbergen May 14, 2019

Member

why do we need to repeat this?

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall May 14, 2019

Author Member

I want to avoid the thread spinning forever, this limits its life to 2 seconds (20x0.1).

This comment has been minimized.

Copy link
@wvanbergen

wvanbergen May 14, 2019

Member

Why do we need the loop in the first place? Would this not work if we only signal it once?

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall May 14, 2019

Author Member

This will only signal the command once - it breaks immediately after. I added this to protect myself when breaking something else in the code - mistyping a path or causing a loop etc. Without some kind of circuit breaker thread.join will never return in cases where the command is not started.

Show resolved Hide resolved test/unit/command_test.rb Outdated
@wvanbergen
Copy link
Member

left a comment

None of my issues are blockers, so feel free to address what you think should be addressed, and merge afterwards.

@JackTLi
Copy link
Member

left a comment

👏

@DazWorrall DazWorrall merged commit 6170a31 into master May 15, 2019

3 checks passed

CLA Contributor License Agreement (CLA) status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@DazWorrall DazWorrall deleted the report-signalled-commands branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.