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

Bug fix find-linked-issue.js for "Schedule Friday" #7104

Merged

Conversation

t-will-gillis
Copy link
Member

@t-will-gillis t-will-gillis commented Jul 13, 2024

Fixes #7080

What changes did you make?

  • Added error catching to find-linked-issue.js at line 11 let matches = text.matchAll(re)
  • Same file, added function comments

Why did you make the changes (we will use this info to test)?

  • Error catching was added because for the last several weeks, the "Schedule Friday" workflow has stopped running while reviewing the timeline for issue 6882. Specific details are on JavaScript bug fix: "Schedule Friday" workflow #7080.
  • The error catching does not change any evaluation, its purpose is to catch the TypeError raised when issue 6882 is evaluated so that the workflow can run to completion.
  • Comments help others to understand the intended actions of the function.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

  • No visual changes to website

Testing results and explanation:

  • Schedule Friday workflow log.

    • The workflow is scanning all flagged issues. Line 4891 (see below) shows that the same TypeError is caught for issue 6882. In the current HfLA "Schedule Friday" workflows, this causes a fatal error, however with the code edits this workflow run (see below) continues to completion as as intended.
    • (Note that line 4906 shows a RequestError. This is due to a token error/ lack of permissions because this workflow is running from a personal repo against issues in the HfLA repo. This is not a concern because the RequestError will not occur when the workflow runs from the HfLA repo.)
    Screenshot 2024-07-14 141727

Add error catching at matchAll() and function comments
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!


From your project repository, check out a new branch and test the changes.

git checkout -b t-will-gillis-sch-fri-bug-fix-7080 gh-pages
git pull https://github.com/t-will-gillis/website.git sch-fri-bug-fix-7080

@github-actions github-actions bot added Bug Something isn't working role: back end/devOps Tasks for back-end developers Complexity: Medium Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly Feature: Refactor GHA Refactoring GitHub actions to fit latest architectural norms size: 3pt Can be done in 13-18 hours labels Jul 13, 2024
@santisecco santisecco self-requested a review July 15, 2024 14:27
@santisecco
Copy link
Member

santisecco commented Jul 17, 2024

Review ETA: 4 PM 7/17/24
Availability: 11-3 PM 7/17/24

@santisecco santisecco requested review from santisecco and removed request for santisecco July 17, 2024 22:40
@santisecco
Copy link
Member

santisecco commented Jul 17, 2024

I was considering reviewing this pull request. I spent a while understanding all the parts of issue #7080 I do agree with adding the error handling but I first would like to ask some questions when the meetings resume.

It's interesting that for some reason the function:

function isLinkedIssue(data, issueNum) {
  return findLinkedIssue(data.source.issue.body) == issueNum
}

Reading the log, in #6882 data.source.issue.body appears to be null. Later function findLinkedIssue(text) in file find-linked-issue.js with the proposed changes, cases in which the input is null or invalid would be handled. But still I can't think of a solution but I would like to note that there is no issue with #6883 in the new project board, but #6881 is present. Maybe that caused things to break somewhere else.

The PR is correct, there's a linked issue, branches are correct and the code is clean. But I would like to have further comprehension of the workflows and other parts of the website before approving.

@t-will-gillis
Copy link
Member Author

Hey @santisecco thank you for the comments. I respect that you would like to know more about this before approving it- if you would like, we can discuss.
Regarding issue 6882, yeah I was not understanding why data.source.issue.body is null for this issue but not for more recent Pre-work issues (the workflow is looking at issues in reverse chronological order). In the current file, text.matchAll(null) raises the TypeError and halts the workflow, which was the reason for adding the error handling.
btw please let me know if you have other ideas about this...

@santisecco
Copy link
Member

@t-will-gillis sorry for the delay. Yeah I saw the loop in add-label.js starting from the last issues to the newest ones. I agree that text.matchAll(null) is causing the error. What I meant previously is that I can't understand why data.source.issue.body would be null. Still I agree that adding the error handling is a solution.

@santisecco santisecco self-requested a review July 22, 2024 17:26
Copy link
Member

@santisecco santisecco left a comment

Choose a reason for hiding this comment

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

Hi @t-will-gillis I agree that adding the error handling is a solution for the TypeError caused. Branches are correct and the code is concise and clear.
Thanks

@t-will-gillis t-will-gillis merged commit e7916ea into hackforla:gh-pages Jul 22, 2024
11 checks passed
@t-will-gillis t-will-gillis deleted the sch-fri-bug-fix-7080 branch July 22, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Complexity: Medium Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly Feature: Refactor GHA Refactoring GitHub actions to fit latest architectural norms role: back end/devOps Tasks for back-end developers size: 3pt Can be done in 13-18 hours
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JavaScript bug fix: "Schedule Friday" workflow
2 participants