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

Move core/local/ExecutionScheduler to execution/Scheduler #2812

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

na--
Copy link
Member

@na-- na-- commented Dec 7, 2022

...and also move and expand the special context wrapper that test.abort() uses to interrupt the test and store the error internally. Its improved capabilities will be required for #1889

This is built on top of #2810 and is also the first step towards addressing #1302. There is still a long way to go before we can move all of the execution-related things out of lib/, mostly because it will be quite tricky to both have a nice module structure and avoid import loops 😞 Even now, parts of execution/ (esp. the tests) still import lib/executor/, while lib/executor imports the parts of lib/ we want to move to execution/... 😭 I worked around the issue this time, though that might not always work as well as it did this time.

In any case, I just made this change to get the process of cleaning up lib/ started. Between this PR and the next one to address #1889, it's much easier to remove the core/ package than it is to cleanly split apart lib/. Now, we have at least a place for these things, even if we can't move everything quite yet.

@na-- na-- added this to the v0.43.0 milestone Dec 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #2812 (08d5acf) into master (a2a5b39) will decrease coverage by 0.35%.
The diff coverage is 77.14%.

❗ Current head 08d5acf differs from pull request most recent head a42bdc7. Consider uploading reports for the commit a42bdc7 to get more accurate results

@@            Coverage Diff             @@
##           master    #2812      +/-   ##
==========================================
- Coverage   76.88%   76.54%   -0.35%     
==========================================
  Files         218      217       -1     
  Lines       16650    16664      +14     
==========================================
- Hits        12802    12755      -47     
- Misses       3041     3092      +51     
- Partials      807      817      +10     
Flag Coverage Δ
ubuntu ?
windows 76.54% <77.14%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
api/v1/status_routes.go 31.25% <ø> (-27.94%) ⬇️
core/engine.go 94.33% <ø> (ø)
lib/execution.go 87.15% <ø> (-2.76%) ⬇️
execution/abort.go 70.37% <70.37%> (ø)
cmd/run.go 72.22% <100.00%> (ø)
execution/scheduler.go 90.07% <100.00%> (ø)
lib/executor/helpers.go 100.00% <100.00%> (+0.97%) ⬆️
lib/netext/tls.go 27.08% <0.00%> (-22.92%) ⬇️
api/v1/metric.go 54.54% <0.00%> (-13.64%) ⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

olegbespalov
olegbespalov previously approved these changes Dec 14, 2022
oleiade
oleiade previously approved these changes Dec 14, 2022
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

Very good work 👍🏻 I'm glad to see the move to a dedicated execution package happen 🎉 🦖 I've left a suggestion to maybe find a more suiting name for the Scheduler as we're at it, but I've really no strong opinion on this, and you should feel free to walk past it 🙇🏻

execution/scheduler.go Show resolved Hide resolved
@na-- na-- force-pushed the refactor-run-status branch 2 times, most recently from 8c360a1 to 4077da3 Compare January 16, 2023 13:24
Base automatically changed from refactor-run-status to master January 24, 2023 07:58
@na-- na-- dismissed stale reviews from oleiade and olegbespalov via ef09f1b January 24, 2023 07:58
We also remove the lib.ExecutionScheduler interface because it only ever had a single implementation and it's unlikely we'll ever need more than that. Distributed execution will be implemented another way and we should not mock the execution scheduler, we should mock the parts it moves (e.g. the Runner and Executors, if needs be).
Copy link
Contributor

@codebien codebien 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 a minor nitpick

execution/scheduler_int_test.go Outdated Show resolved Hide resolved
@oleiade oleiade self-requested a review January 24, 2023 09:00
oleiade
oleiade previously approved these changes Jan 24, 2023
@na-- na-- merged commit 1d99b0b into master Jan 24, 2023
@na-- na-- deleted the move-things-to-execution branch January 24, 2023 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants