search

EESSI / eessi-bot-software-layer

Bot to help with requests to add software installations to the EESSI software layer
GNU General Public License v2.0
0 stars 18 forks source link

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

Closed Neves-P closed 9 months ago

Neves-P commented 9 months ago

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.

Neves-P commented 9 months ago

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

Neves-P commented 9 months ago

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: https://github.com/Neves-Bot/software-layer/pull/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 :)

Neves-P commented 9 months ago

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 👍