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

Always write run_results.json #7539

Merged
merged 12 commits into from
May 9, 2023

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented May 6, 2023

resolves #7302
resolves #7448

Description

In an effort to ensure run_results.json is always available regardless of whether or not the run was interrupted or terminated we now write them as each node finishes and once more at the end of the run.

Checklist

@cla-bot cla-bot bot added the cla:yes label May 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

self.previous_state: Optional[PreviousState] = None
self.run_count: int = 0
self.started: float = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.started is the only new value here. The rest of the changes are just sorting things for readability.

@iknox-fa iknox-fa marked this pull request as ready for review May 9, 2023 14:47
@iknox-fa iknox-fa requested a review from a team May 9, 2023 14:47
@iknox-fa iknox-fa requested review from a team as code owners May 9, 2023 14:47
@iknox-fa iknox-fa requested review from stu-k, peterallenwebb, emmyoop and VersusFacit and removed request for a team, peterallenwebb, emmyoop and VersusFacit May 9, 2023 14:47
Copy link
Contributor

@stu-k stu-k left a comment

Choose a reason for hiding this comment

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

Just a few comments. I don't have any better suggestions on the testing strategies though, apologies. It may be a good idea to get a review from someone on language as well since results/writing is technically their domain, I think. Feel free to re-request a review when my comments have been resolved!

core/dbt/task/runnable.py Outdated Show resolved Hide resolved
core/dbt/task/runnable.py Outdated Show resolved Hide resolved
core/dbt/task/runnable.py Show resolved Hide resolved
Copy link
Contributor

@stu-k stu-k left a comment

Choose a reason for hiding this comment

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

Comments addressed!

Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

LGTM!

@iknox-fa iknox-fa merged commit dffbb6a into main May 9, 2023
@iknox-fa iknox-fa deleted the iknox/CT-2387-GH-7302-always-write-run-results branch May 9, 2023 18:46
@aranke aranke mentioned this pull request May 9, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants