Skip to content
This repository has been archived by the owner on Jun 21, 2024. It is now read-only.

cherry-picked PR #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

cherry-picked PR #14

wants to merge 4 commits into from

Conversation

tom-borcin
Copy link
Contributor

Added functionality to leave comment and add [Merged] keyword to cherry-picked pull request. It is triggered by push event and searches the commit message for pull request number to be handled.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

I have quite a few comments, but I'm very excited to see this feature arriving. :)

sync_issues_to_jira/push_event.py Outdated Show resolved Hide resolved
@@ -52,6 +53,11 @@ def main():
with open(os.environ['GITHUB_EVENT_PATH'], 'r') as f:
event = json.load(f)
print(json.dumps(event, indent=4))

# Check if it's a push event
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this would be easier to test and maintain as a standalone action. There isn't any conceptual relationship between this functionality and JIRA, and the script doesn't use any functions from the jira sync code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to refactor the github-actions so it is standalone action. Unfortunately this requires much more effort than I anticipated initially and I suggest we address that in new pull request.

sync_issues_to_jira/push_event.py Show resolved Hide resolved
sync_issues_to_jira/push_event.py Outdated Show resolved Hide resolved
new_title = '[Merged] ' + original_title
pull_request.edit(title=new_title)
# Thank contributor for opening pull request. Let them know we didn't throw it away
pull_request.create_issue_comment('The pull request has been cherry-picked, the commit is linked above.\
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes we actually do "close" a PR by doing something different to what the author originally intended, and with a totally different commit. We try to avoid this, but it's necessary sometimes.

Ideally this action would check if the commit author is the same as the author of any of the commits in the PR branch, and only continue if it is. However this is probably a rare enough occasion that you can consider it a "nice to have". :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion. I have noted this feature request and will implement it in the future.



def parse_commit_message(commit_message):
# Regex matches numbers that come after Fix, fix, Fixed, fixed, Fixes, fixes keyword followed by any
Copy link
Contributor

Choose a reason for hiding this comment

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

Our internal guide suggests writing "Merges ..." as well (which isn't a GitHub keyword) and then manually telling the author their PR has been cherry-picked. This is something I originally did a lot as it seemed more polite. However, in recenttimes most people (including me) don't do this any more and they will write "Closes" or "Fixes".

I will send you a link to the page, up to you if you prefer to update the script to make the original workflow convenient or update the workflow to match the current behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update workflow after this functionality is live in github-actions.

@tom-borcin tom-borcin requested a review from igrr August 23, 2021 12:56
@igrr
Copy link
Member

igrr commented Aug 25, 2021

@tom-borcin I would ask to address the two remaining requests, if you can:

  • move this functionality into a standalone action (comment-on-push or similar?)
  • handle the situation when the PR is closed by a commit unrelated to the PR.

I think the easiest way to handle the latter is to still require Merges <PR URL> comment in the commit message if the PR has been cherry-picked. The workflow would be:

  1. If the PR is merged by cherry-picking, append Merges <PR URL> to the commit message. When the commit is merged to the main branch, Github will not automatically close the PR. Your action recognizes the word Merges in the commit message, checks that the PR is still open, leaves a comment thanking the contributor, changes the title, and closes the PR.
  2. If the PR is being closed by an unrelated commit (e.g. we implement something that solves the problem which PR addressed, but in a different way), the developer uses Closes <PR URL> in the commit message instead, same as they do when closing issues. When the commit is merged into the main branch, Github automatically closes the PR. The action doesn't need to be leave a comment in this case, and doesn't need to change the title.

Besides that, there is one more thing we can address by this comment-on-push action. It is related to the backporting workflow. Currently we add Closes <issue URL> to the commit message when fixing the issue on the main branch. When the commit is merged, Github automatically closes the issue, leaving a note in the issue UI such as @espressif-bot closed this in de0a24e 1 day ago. When the commit is backported to one of the release branches, typically the commit message is not modified, and the commit still references the issue URL. However when such commit is merged into one of the release branches, Github no longer adds any comments into the PR. This makes it harder for issue reporters to track whether the fix is already available in a particular release or not. Sometimes we leave summary comments to tell which commits have fixed the issue (such as espressif/esp-idf#5990 (comment)), but this is not automated and isn't applied consistently.

I think we can use the same approach, triggering an action on a push event (this time for release branches only), detecting Closes in the commit message, and leaving the comment like This issue was fixed on branch release/X.Y with commit <ID>.

One idea I have is to let the action accept several arguments:

  • Regex to search for.
  • Template of the comment to leave. The template may contain Python formatting placeholders such as {commit_id} or {branch}, which will be expanded to the actual commit ID or branch name when generating the message.
  • Whether to close the issue/PR (bool)
  • Whether to change issue/PR title (optional string to add)

This way we can use the same action in both cases, specifying different arguments.

@tomassebestik
Copy link
Member

@tom-borcin Can we close this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants