-
Notifications
You must be signed in to change notification settings - Fork 104
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
Remove hook manager #4012
Remove hook manager #4012
Conversation
c020476
to
501b2ea
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.
nicely done, not much to comment, besides the little things with the test assertion and the freeing : )
@@ -50,6 +53,7 @@ ert_workflow_list_alloc_empty(const subst_list_type *context) { | |||
workflow_list->joblist = workflow_joblist_alloc(); | |||
workflow_list->context = context; | |||
workflow_list->last_error = NULL; | |||
workflow_list->hook_workflow_list = vector_alloc_new(); |
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.
you're planning to reimplement this in python when simplifying the workflow list, so i understand if this is not a priority. just wanted to mention that freeing the hook_workflow_list
might be reasonable.
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.
Fixed!
continue | ||
|
||
workflow = hook_workflow.getWorkflow() | ||
for workflow in workflow_list.get_workflows_hooked_at(runtime): |
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.
nice and elegant. i like
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.
Thanks :)
@@ -648,4 +647,6 @@ def test_that_workflow_run_modes_can_be_selected(tmp_path, run_mode): | |||
f"HOOK_WORKFLOW SCRIPT {run_mode}\n" | |||
) | |||
res_config = ResConfig(str(test_user_config)) | |||
assert res_config.hook_manager[0].getRunMode() == run_mode | |||
assert ( | |||
len(list(res_config.ert_workflow_list.get_workflows_hooked_at(run_mode))) == 1 |
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.
much nicer!
@@ -71,7 +71,6 @@ def test_site_config_hook_workflow(monkeypatch, tmp_path): | |||
monkeypatch.setenv("ERT_SITE_CONFIG", site_config_filename) | |||
|
|||
res_config = ResConfig(user_config_file=test_config_filename) | |||
assert len(res_config.hook_manager) == 1 |
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.
you didn't replace this assertion with something equivalent, as far as i can see - you deemed it unnecessary to remain in the test?
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.
I added back a similar assertion just in case :)
501b2ea
to
a53e8f5
Compare
a53e8f5
to
000c47c
Compare
Issue
Resolves one of the items of #3986
Approach
The only part of hook manager that is used it the hook_workflow_list. This is really a map from workflow (time at which the workflow should run) to run_mode, telling ert when to run the workflow. Its probably a good idea to keep this information together with the workflows, ie. in
ert_workflow_list
(which is next on the list for simplification, and now that hook manager is gone, ert_workflow_list can be reimplemented in python).Pre review checklist
Adding labels helps the maintainers when writing release notes. This is the list of release note labels.