Add a comment when downloading and merging from a PR fails via `comment_download_pr()` #248

Neves-P · 2024-02-11T16:30:57Z

Adds a new function comment_download_pr() that takes the exit codes from download_pr() to give an helpful message via a GH comment on an open PR.

The aim is to mostly handle situations where a merge conflict due to an upstream merge prevents the git apply step in download_pr() from working. At this stage of download_pr(), the recent changes (a patch) are downloaded and git apply is used. When a merge conflict is present, git apply fails with an error and which not reported by the bot and made visible to the user.

For this change, I also explicitly pass the error codes out of download_pr() to be able to expose the error message to the user and handle the errors.

To be done in the future: add unit tests to the new functionality. I have started, but not managed to finish the mocking and testing framework for this function. Adding this PR so we have have this functionality earlier.

This PR addresses #212.

Merge bot changes since we have software.eessi.io

Neves-P · 2024-02-11T16:33:02Z

I'll handle all the linter comments tomorrow (12/02)

trz42

Nice work and more than just checking the result of git apply.

For some print statements, log might be better.

The calling function may not easily know which step (cloning the repo, branching, downloading the diff or applying the diff) failed. If the result would contain a 4th item one might include that information and use it when checking the exit code. Thus the note to the user could be adjusted and also the "Tip". Alternatively, one might only check the result of git apply.

Would the comment be created for all potential jobs or just once (because an exception is raised)? If multiple comments would be created, might be ok for now as it is already an improvement.

trz42 · 2024-02-11T18:05:32Z

tasks/build.py

@@ -346,21 +346,60 @@ def download_pr(repo_name, branch_name, pr, arch_job_dir):
    # - 'curl' diff for pull request
    # - 'git apply' diff file
    git_clone_cmd = ' '.join(['git clone', f'https://github.com/{repo_name}', arch_job_dir])
-    clone_output, clone_error, clone_exit_code = run_cmd(git_clone_cmd, "Clone repo", arch_job_dir)
+    print(f'cloning with command {git_clone_cmd}')


This prints to stdout. Should it maybe go to a log file?

Absolutely, I've changed this and other occurrences to use log().

trz42 · 2024-02-11T18:06:17Z

tasks/build.py


    git_checkout_cmd = ' '.join([
        'git checkout',
        branch_name,
    ])
+    print(f'checking out with command {git_checkout_cmd}')


This prints to stdout. Should it maybe go to a log file?

trz42 · 2024-02-11T18:07:05Z

tasks/build.py


    curl_cmd = f'curl -L https://github.com/{repo_name}/pull/{pr.number}.diff > {pr.number}.diff'
-    curl_output, curl_error, curl_exit_code = run_cmd(curl_cmd, "Obtain patch", arch_job_dir)
-
+    print(f'curl with command {curl_cmd}')


This prints to stdout. Should it maybe go to a log file?

trz42 · 2024-02-11T18:07:42Z

tasks/build.py

    git_apply_cmd = f'git apply {pr.number}.diff'
-    git_apply_output, git_apply_error, git_apply_exit_code = run_cmd(git_apply_cmd, "Apply patch", arch_job_dir)
+    print(f'git apply with command {git_apply_cmd}')


This prints to stdout. Should it maybe go to a log file?

trz42 · 2024-02-11T18:17:57Z

tasks/build.py

+        download_comment = (f"`{download_pr_error}`"
+                           f"\nUnable to download or merge changes between the source branch and the destination branch.\n"
+                           f"\nTip: This can usually be resolved by syncing your branch and resolving any merge conflicts.")
+        download_comment = pr_comments.create_comment(repo_name=base_repo_name, pr_number=pr.number, comment=download_comment)


This creates a new comment (for each job that failed), right? So we might get many new comments. Wonder if it could be possible to pass the result back to the calling function such that it is included in the responses to the bot: build command(s)?

Edit: Maybe just one comment is added because an exception is raised for the first error that happened?

That's a good point. I actually meant for it to just comment once and stop with the exception, but I did not actually verify that it works that way because when I was playing around with it I was only building for one target: Neves-Bot/software-layer#12 (comment)

In that case of course the bot could only write one comment for that one target, so I'm completely sure the raising the error is enough 🤔

Neves-P · 2024-02-12T16:23:07Z

Nice work and more than just checking the result of git apply.

Thank you! I've address the other change requests individually.

The calling function may not easily know which step (cloning the repo, branching, downloading the diff or applying the diff) failed. If the result would contain a 4th item one might include that information and use it when checking the exit code. Thus the note to the user could be adjusted and also the "Tip". Alternatively, one might only check the result of git apply.

Indeed, that's a great point, I've tried addressing that just now, where the error reporting and tip are handled depending on where the error occurs in download_pr(). This required returning a new error_stage variable to inform comment_download_pr() of what to do. There might be a more clever way to do this, but I couldn't think of anything simple at first thought.

One very important note: I am in the process of reinstalling the bot on our HPC and setting it up to build for all the CPU targets we need in Groningen, so until that is done I can't test this at the moment as I was doing here: Neves-Bot/software-layer#12. I know the code before the latest commits was working, but I can't say the same for this revision. I hope to have some time do finish that this week and then I'll already test this code. Unless there is a way of testing this outside of production code that I'm not aware of, it may be wise to wait on the merge until then :)

trz42

Looks great. Just a small suggestion on how to use string constants, e.g., define a variable at the beginning, then use that variable.

The text snippets that are used to print error messages could also be made configurable. That would allow us to change them in the configuration file (instead of changing them in the bot code). What do you think, would this make sense or would it be overkill here?

trz42 · 2024-02-13T08:35:06Z

tasks/build.py

+        git_clone_cmd, "Clone repo", arch_job_dir, raise_on_error=False
+        )
+    if clone_exit_code != 0:
+        error_stage = 'git clone'


Very good. Minor suggestion

Suggested change

error_stage = 'git clone'

error_stage = ERROR_GIT_CLONE

and at the beginning (lines 34-79) add something like

ERROR_GIT_CLONE = 'git clone'

Done, for every occurrence of error_stage.

trz42 · 2024-02-13T08:35:54Z

tasks/build.py

+    """
+    if download_pr_exit_code != 0:
+        fn = sys._getframe().f_code.co_name
+        if error_stage == 'git clone':


Use globally defined variable ERROR_GIT_CLONE, e.g.,

Suggested change

if error_stage == 'git clone':

if error_stage == ERROR_GIT_CLONE:

Moved string constants to globally defined variables in header.

Neves-P · 2024-02-14T15:41:16Z

Thanks for the review! I've tried addressing them all, the string constants are globally defined at the top now and I've moved the text snippets to the cfg file. Let me know if something isn't right or if I should make more changes 👍

trz42

Really nice work @Neves-P !!! Apologies for so many requests for changes.

Neves-P and others added 11 commits November 15, 2023 17:27

download_pr error handling

81a964c

Improve message and fix typo

db69f09

Yet more comment formatting

b688574

More comment improvements

1b65a3a

Tiny formatting change

e3657a9

Comment changes

e7c6d65

Merge pull request #1 from EESSI/develop

b661632

Merge bot changes since we have software.eessi.io

Merge branch 'EESSI:develop' into develop

d00df55

Revert changes to test_task_build.py

f1ac9a4

Revert changes to test_task_build.py

5a96c4f

Revert changes to test_task_build.py

b82cc0b

trz42 requested changes Feb 11, 2024

View reviewed changes

Neves-P added 3 commits February 12, 2024 15:56

Replace print() with log() where appropriate

e35f462

Lint code

d92e1da

Handle each error stage individually

3213fbe

trz42 requested changes Feb 13, 2024

View reviewed changes

Neves-P added 2 commits February 14, 2024 16:01

Use globaly defined string constants for errors

570217f

Move text snippets to cfg_file

fd6cda1

trz42 approved these changes Feb 14, 2024

View reviewed changes

trz42 merged commit 75893a4 into EESSI:develop Feb 14, 2024
7 checks passed

This was referenced Feb 27, 2024

release notes for v0.4.0 #258

Merged

release v0.4.0 #259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a comment when downloading and merging from a PR fails via `comment_download_pr()` #248

Add a comment when downloading and merging from a PR fails via `comment_download_pr()` #248

Neves-P commented Feb 11, 2024 •

edited

Loading

Neves-P commented Feb 11, 2024

trz42 left a comment

trz42 Feb 11, 2024

Neves-P Feb 12, 2024

trz42 Feb 11, 2024

trz42 Feb 11, 2024

trz42 Feb 11, 2024

trz42 Feb 11, 2024 •

edited

Loading

Neves-P Feb 12, 2024

Neves-P commented Feb 12, 2024

trz42 left a comment

trz42 Feb 13, 2024

Neves-P Feb 14, 2024

trz42 Feb 13, 2024

Neves-P Feb 14, 2024

Neves-P commented Feb 14, 2024

trz42 left a comment

	if error_stage == 'git clone':
	if error_stage == ERROR_GIT_CLONE:

Add a comment when downloading and merging from a PR fails via comment_download_pr() #248

Add a comment when downloading and merging from a PR fails via comment_download_pr() #248

Conversation

Neves-P commented Feb 11, 2024 • edited Loading

Neves-P commented Feb 11, 2024

trz42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trz42 Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neves-P commented Feb 12, 2024

trz42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neves-P commented Feb 14, 2024

trz42 left a comment

Choose a reason for hiding this comment

Add a comment when downloading and merging from a PR fails via `comment_download_pr()` #248

Add a comment when downloading and merging from a PR fails via `comment_download_pr()` #248

Neves-P commented Feb 11, 2024 •

edited

Loading

trz42 Feb 11, 2024 •

edited

Loading