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

Make possible for the cloud output to stop the engine #1965

Merged
merged 7 commits into from
Apr 16, 2021
Merged

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Apr 14, 2021

This does add an interface that can be implemented by other outputs as well.

fixes #1880

TODOs:

  • Figure out a good name. I kept changing what I think this should be called through the writing of this and am not certain I like any of the names much ... I would argue that the best one is StopExecutionOnOutputSignal as it makes it more accurate that we will over stop if the output couldn't be started, and that this is a signal from the output to stop not just that there was some error. Unfortunately, it's a bit of a mouthful
  • Figure out how to write an integration test of this ... I have tested it manually and it works (and takes 3 minutes to test) but it will be nice to have a full integration test about this.
  • Print which output signaled to close

Related to the last point, I decided to have an error be "returned" not just to call some function is so that we can always print it in some good known format instead of it being a responsibility of the output.

@mstoykov mstoykov added this to the v0.32.0 milestone Apr 14, 2021
@mstoykov mstoykov requested review from imiric and na-- April 14, 2021 15:51
@mstoykov mstoykov changed the title Fix/1880 Add the ability for an output to stop the execution, specifically the cloud one Apr 14, 2021
@na--
Copy link
Member

na-- commented Apr 14, 2021

I'll get back to you on the names later, but please make this a cloud output option, not a runtime option.

@codecov-io
Copy link

codecov-io commented Apr 14, 2021

Codecov Report

Merging #1965 (f59b2ee) into master (311b905) will decrease coverage by 0.05%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1965      +/-   ##
==========================================
- Coverage   71.48%   71.43%   -0.06%     
==========================================
  Files         183      183              
  Lines       14247    14257      +10     
==========================================
  Hits        10184    10184              
- Misses       3432     3442      +10     
  Partials      631      631              
Flag Coverage Δ
ubuntu 71.35% <40.00%> (-0.05%) ⬇️
windows 71.04% <40.00%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
core/engine.go 83.25% <0.00%> (-1.88%) ⬇️
output/cloud/output.go 81.15% <66.66%> (-0.13%) ⬇️
cloudapi/config.go 85.54% <100.00%> (+0.35%) ⬆️
lib/executor/ramping_vus.go 93.80% <0.00%> (-1.24%) ⬇️
lib/executor/vu_handle.go 94.28% <0.00%> (-0.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 311b905...f59b2ee. Read the comment docs.

This does add an interface that can be implemented by other outputs as
well.

fixes #1880
@mstoykov mstoykov changed the title Add the ability for an output to stop the execution, specifically the cloud one Make possible for the cloud output to stop the engine Apr 14, 2021
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

Re: the name, StopRunWithError sounds better to me than StopExecutionOnOutputSignal, as "signal" has the Unix signal connotation. Though I would add Engine to it since that's what happens, so StopEngineRunWithError?

And I agree with the need for an integration test, but it could be done in a follow up PR.

core/engine.go Outdated Show resolved Hide resolved
output/types.go Outdated Show resolved Hide resolved
imiric
imiric previously approved these changes Apr 15, 2021
core/engine.go Outdated Show resolved Hide resolved
output/types.go Outdated Show resolved Hide resolved
Co-authored-by: na-- <n@andreev.sh>
na--
na-- previously approved these changes Apr 16, 2021
@mstoykov mstoykov merged commit 29075af into master Apr 16, 2021
@mstoykov mstoykov deleted the fix/1880 branch April 16, 2021 14:08
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.

Stop k6 run --out cloud test when it gets aborted from the web app
4 participants