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

Only suppress diff for run_if when it is ALL_SUCCESS #3454

Merged
merged 5 commits into from
Apr 12, 2024
Merged

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Apr 11, 2024

Changes

This PR introduces the SetSuppressDiffWithDefault which allows us to suppress diff in one of the most common cases. The first use of it is for run_if.

When a run_if is not defined for a task, the platform assigns it a default of ALL_SUCCESS. This PR fixes the suppress diff behaviour to only suppress the diff if the platform returns the default value, and not suppress it in all cases.

This PR treats one of the symptoms of job tasks being reordered during diff computation, and tasks that do not have a run_if defined ending up with one in the final job.

Tests

Unit tests and manually. Task reordering during diff computation no longer incorrectly sets run_if.

@shreyas-goenka shreyas-goenka requested review from a team as code owners April 11, 2024 18:05
@shreyas-goenka shreyas-goenka requested review from mgyucht and removed request for a team and mgyucht April 11, 2024 18:05
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@alexott
Copy link
Contributor

alexott commented Apr 11, 2024

Are integration tests passing?

@shreyas-goenka
Copy link
Contributor Author

Integration tests are OK. The tests that are failing are unrelated to the change are also failing on main.

@shreyas-goenka shreyas-goenka added this pull request to the merge queue Apr 12, 2024
@shreyas-goenka shreyas-goenka removed this pull request from the merge queue due to a manual request Apr 12, 2024
@shreyas-goenka shreyas-goenka added this pull request to the merge queue Apr 12, 2024
@shreyas-goenka shreyas-goenka removed this pull request from the merge queue due to a manual request Apr 12, 2024
@mgyucht mgyucht added this pull request to the merge queue Apr 12, 2024
@mgyucht mgyucht removed this pull request from the merge queue due to a manual request Apr 12, 2024
@shreyas-goenka shreyas-goenka requested a review from mgyucht April 12, 2024 13:24
@shreyas-goenka shreyas-goenka changed the title Do not suppress diff for run_if if it's not the default value Only suppress diff for run_if when it is ALL_SUCCESS Apr 12, 2024
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Amazing!

common/customizable_schema.go Outdated Show resolved Hide resolved
@mgyucht mgyucht enabled auto-merge April 12, 2024 13:29
@mgyucht mgyucht added this pull request to the merge queue Apr 12, 2024
Merged via the queue into main with commit 1bf97ef Apr 12, 2024
5 checks passed
@mgyucht mgyucht deleted the fix/run_if_suppress branch April 12, 2024 13:35
@tanmay-db tanmay-db mentioned this pull request Apr 16, 2024
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.

3 participants