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

[CI] Attempts to avoid break when diff consists of one whitespace only #244

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

CarlosNihelton
Copy link
Collaborator

@CarlosNihelton CarlosNihelton commented Jul 25, 2022

Sometimes we fail to follow the formatting convention due to whitespaces and new lines only. In such cases CI fails to generate GitHub PR comments with the following validation error message:

{"message":"Unprocessable Entity","errors":["Pull request review thread start line must precede the end line."],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-review-for-a-pull-request"}
Posting review comments failed with error code: 422

It seems that the start_line and line fields of a comment object must not be the same otherwise, the invariant start line must precede the end line will not hold.

            review_comments.append(
                {
                    "path": file_path,
                    "start_line": start_line,
                    "line": line_number,
                    "side": "RIGHT",
                    "body": review_comment_body,
                }
            )

This is somewhat an educated guess, because the comment object documentation is not very clear on that.

This PR aims to make sure that invariant holds by not including the start_line field when it's equal to line and further manipulation of the diff hunk target line contents to ensure we always ends with a new line if not empty.

Only commit e36ea7f is relevant code wise.

In order to demonstrate that this is behaving as expected, commits 8552fcc and 8c47a0c were created simulating the typical formatting errors that caused crashes before. The results are manifested in the github-actions-bot comments that can be seen below. The last commit just reverts those two.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

@ubuntu ubuntu deleted a comment from github-actions bot Jul 25, 2022
@CarlosNihelton CarlosNihelton force-pushed the fix-whitespace-ci branch 2 times, most recently from 5ecee06 to 124a174 Compare July 25, 2022 16:47
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

@ubuntu ubuntu deleted a comment from github-actions bot Jul 25, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

Comment on lines 102 to 105




Copy link
Contributor

Choose a reason for hiding this comment

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

Clang-format suggestion below:

Suggested change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We had previous crashes due to code changes like that.

}

return false;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Clang-format suggestion below:

Suggested change
return false;
return false;

This reverts commit 8c47a0c.

Revert "Pokes the CI"

This reverts commit 8552fcc.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

@@ -90,7 +90,7 @@ namespace Oobe
} // namespace.

SplashEnabledStrategy::SplashEnabledStrategy() : splashExePath{splashPath()}
{ }
{ }
Copy link
Contributor

Choose a reason for hiding this comment

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

Clang-format suggestion below:

Suggested change
{ }
{
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is exactly equal to the source change that caused the last crash.

@CarlosNihelton CarlosNihelton marked this pull request as ready for review July 25, 2022 18:51
Copy link
Collaborator

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

Great!

btw is this code unique to Ubuntu WSL? If so, no further comments. Otherwise, we should also patch upstream.

@CarlosNihelton
Copy link
Collaborator Author

Great!

btw is this code unique to Ubuntu WSL? If so, no further comments. Otherwise, we should also patch upstream.

That's unique to Ubuntu WSL. I think this piece that generates comments from a diff has a very interesting potential, but for now it's only used here.

@CarlosNihelton CarlosNihelton merged commit d2dd984 into main Jul 26, 2022
@CarlosNihelton CarlosNihelton deleted the fix-whitespace-ci branch July 26, 2022 09:56
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.

None yet

2 participants