search

BigSamu / OpenSearch_Changelog_Workflow

A reusable workflow for automating changelog and release notes generation processes for OpenSearch repos
Apache License 2.0
0 stars 4 forks source link

Implement ability to add error comments to PRs #11

Closed JohnathonBowers closed 9 months ago

JohnathonBowers commented 9 months ago

Description

These changes provide the initial implementation of the ability for error messages to be added as comments to PRs. The logic for parsing changelog entries has been nested within a try-catch block. Any errors thrown during the parsing of the "Changelog" section of the PR description — which are the errors the contributor should know about and can fix — are passed as arguments to the postPRComment function in githubUtils.js. This function uses a map of error constructors and generator functions to derive the proper message to add as a PR comment.

In order to manually test this feature, I believe we would need to merge the code into main. However, I wanted to hold off on doing that so that @BigSamu can take a look at the general setup before we move to manual testing.

Issues

Addresses Issue #2

BigSamu commented 9 months ago

@JohnathonBowers,

Let's take a look for this after solving my comments for PR #8. With that code more modularized, we can get back to this.

JohnathonBowers commented 9 months ago

@BigSamu , I finished modularizing the code in index.js and implementing the PR comment feature. Let me know your thoughts!

BigSamu commented 9 months ago

@BigSamu , I finished modularizing the code in index.js and implementing the PR comment feature. Let me know your thoughts!

I just checked you did the suggested modularization for index.js in this PR. So, as I mentioned to you in one message in Slack, I recommend, in terms of organization for future work, that specific changes should be related to the PRs where code reviews are done. In this case, the suggestion was in PR #8.

Either way, you can mention in PR #8 in this comment that this suggestion was introduced in PR #11 or move this update to PR #8. Does it make sense what I say? It is to keep consistency.

Of course, I want to hear your opinion. But if this work continues to get bigger, it is better to have everything in order regarding the changes we introduce and suggestions that can come from other reviewers.

JohnathonBowers commented 9 months ago

@BigSamu , I finished modularizing the code in index.js and implementing the PR comment feature. Let me know your thoughts!

I just checked you did the suggested modularization for index.js in this PR. So, as I mentioned to you in one message in Slack, I recommend, in terms of organization for future work, that specific changes should be related to the PRs where code reviews are done. In this case, the suggestion was in PR #8.

Either way, you can mention in PR #8 in this comment that this suggestion was introduced in PR #11 or move this update to PR #8. Does it make sense what I say? It is to keep consistency.

Of course, I want to hear your opinion. But if this work continues to get bigger, it is better to have everything in order regarding the changes we introduce and suggestions that can come from other reviewers.

I agree. My apologies. I referenced this PR in PR #8 so that we have a record of where the change you suggested was implemented.

JohnathonBowers commented 9 months ago

I just deleted files fetchData.js, apiModule.js, and fetchData.test.js in your branch. They are not required any more. The main branch don't have this files either (I removed them this morning)

Sounds good. Thank you!

BigSamu commented 9 months ago

@BigSamu , I finished modularizing the code in index.js and implementing the PR comment feature. Let me know your thoughts!

I just checked you did the suggested modularization for index.js in this PR. So, as I mentioned to you in one message in Slack, I recommend, in terms of organization for future work, that specific changes should be related to the PRs where code reviews are done. In this case, the suggestion was in PR #8. Either way, you can mention in PR #8 in this comment that this suggestion was introduced in PR #11 or move this update to PR #8. Does it make sense what I say? It is to keep consistency. Of course, I want to hear your opinion. But if this work continues to get bigger, it is better to have everything in order regarding the changes we introduce and suggestions that can come from other reviewers.

I agree. My apologies. I referenced this PR in PR #8 so that we have a record of where the change you suggested was implemented.

For the modularization, the suggestion is not quite followed. Try to review again. The idea is have index.js as clean as possible and modularize as much as possible for isolation and testing.