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

Fix bug GHA "Schedule Thursday 1100" (inactive members) 4768 #5158

Conversation

t-will-gillis
Copy link
Member

@t-will-gillis t-will-gillis commented Aug 7, 2023

Fixes #4768

What changes did you make?

  • Since the current schedule-thu-1100.yml was scheduled to run on the first Thursday of the month, this action has been revised/ renamed to schedule-monthly.yml and now is scheduled to run on the first day of the month.
  • Because of this change, there no longer is a schedule-thu-1100.yml file.
  • Note that there is an existing action (Fix bug that prevents GHA "Schedule Monthly" from running  #4788) with the same name. This action/ file is renamed schedule-monthly-PREV.yml. This action is not/ has not been running either. The renaming is intended to be temporary because the plan is to explore how to integrate the other action's steps into this action.
  • The "Get current date" step in the .yml file was deleted because a.) the code is deprecated and b.) it is unnecessary. (The previous file used this step to cause the automation to run only on the first Thursday of the month.)
  • The working-directory in the step "Install npm dependencies" is refactored to the correct directory i.e. ./github-actions/trigger-schedule/github-data. (This was one of the reasons the existing GHA has not worked- this refactor fixes the action.)
  • The node-version is updated to version 18. (The previous version 14 is incompatible with the current modules.)
  • The run path in the step "Trim members" is refactored to the correct directory and the contributors-data.js is moved to the same directory as above. (This was another one of the reasons the existing GHA was not working. The @octokit/rest module cannot be found unless the js file resides in the same directory as the package.json file.)
  • Since this js file is moved, the trim-contributors directory is empty and so is deleted.
  • Additional functionality has been added to contributors-data.js:
    • The automatic removal of members now occurs after two months of inactivity.
    • Members who are one month inactive are placed on list so that they can be contacted. (For a future PR, an automatic notification should be sent to these members to give them a chance to become active again before deletion= See "FUTURE revisions/ additions" below)
  • Since files have been renamed and moved, the "Files changed" link above will not give a useful comparison of what is different. See the 'Diffchecker' screenshots below.

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

  • Changes were made primarily to get this action working again (it has been failing for over a year due to misdirected file paths.)
  • Other reasons are given in the text above.

FUTURE revisions/ additions are detailed in ER #5163

Screenshots of Proposed Changes Of The Website

Diffchecker for new `schedule-monthly.yml`

Screenshot 2023-08-07 at 14-53-59 Computed Diff - Diff Checker

Diffchecker for `contributors-data.js`

Screenshot 2023-08-07 at 14-20-30 contributors-data js - Diff Checker
Screenshot 2023-08-07 at 14-20-50 contributors-data js - Diff Checker
Screenshot 2023-08-07 at 14-21-10 contributors-data js - Diff Checker
Screenshot 2023-08-07 at 14-21-29 contributors-data js - Diff Checker
Screenshot 2023-08-07 at 14-21-43 contributors-data js - Diff Checker
Screenshot 2023-08-07 at 14-22-48 contributors-data js - Diff Checker

@github-actions
Copy link

github-actions bot commented Aug 7, 2023

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-fix-bug-sch-thursday-inactive-4768 gh-pages
git pull https://github.com/t-will-gillis/website.git fix-bug-sch-thursday-inactive-4768

@github-actions github-actions bot added Bug Something isn't working role: back end/devOps Tasks for back-end developers Complexity: Large time sensitive Needs to be worked on by a particular timeframe 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 Aug 7, 2023
@t-will-gillis t-will-gillis added the Draft Issue is still in the process of being created label Aug 7, 2023
@github-actions github-actions bot removed the Draft Issue is still in the process of being created label Aug 7, 2023
add misc. explicit semicolons
@t-will-gillis t-will-gillis added the Draft Issue is still in the process of being created label Aug 8, 2023
@github-actions github-actions bot removed the Draft Issue is still in the process of being created label Aug 8, 2023
@adrianang adrianang requested a review from mademarc August 9, 2023 02:27
@LRenDO
Copy link
Member

LRenDO commented Aug 9, 2023

Hi @mademarc! When you have a minute, please add your ETA and Availablity. Thanks!

@mademarc
Copy link
Member

Review ETA: 8/11/2023
Availability: 5:11PM

mademarc
mademarc previously approved these changes Aug 12, 2023
Copy link
Member

@mademarc mademarc left a comment

Choose a reason for hiding this comment

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

Hey @t-will-gillis the format also the code changes on the line 5 & 8 on the yml file is done correctly and @LRenDO the ETA and Availability is done.

@one2code one2code self-requested a review August 12, 2023 03:25
@one2code
Copy link
Member

ETA: 8/12/23

@one2code
Copy link
Member

Just wanted to comment and say that I have been reviewing this pull request, but due to the complexity and amount of file changes / line changes, I may have to split this one into chunks. I know the challenges that we have been facing with the various schedule files due to things such as deprecation, breaking changes on updates, or changes to how the schedules and triggers need to be setup in the latest updates to avoid breaking functionality. With that said, I want to take my time with this and make sure I can verify under testing the latest changes, with the understanding that this is one key step in solving several other related issues. Likewise, if I were to miss something in testing, then that would create a domino effect, making the other related issues difficult to solve for.

Thank you for your patience as I continue to delve deep on this.

@LRenDO
Copy link
Member

LRenDO commented Aug 23, 2023

Hi @yujioshiro! Thanks for volunteering to review this issue! When you have a minute, please add your ETA and Availability. Thanks!

@LRenDO LRenDO self-requested a review August 24, 2023 20:19
@LRenDO
Copy link
Member

LRenDO commented Aug 24, 2023

Hi @t-will-gillis! I went to review this and I believe I don't have correct permissions to access HFLA Org level information using the API. I get back the repo level information (in fetchContributors) just fine when I run the test, but the org level information (in fetchTeamMembers) gives a 404 response which seems like it is likely a permissions error. See GitHub doc. Did you run into this issue at all when running the GHA manually? I was wondering if your maintainer role grants the right permissions.

That said, I did read through the code and PR and it looks like you did a great job refactoring and solving the bug! I particularly appreciated the explanation and breakdown of what needed to be done to fix the issue in your PR. The inclusion of the diffs was very helpful as well since many things were moved and deleted. It was also useful to see the ER with the other issues that need to be addressed including adding the notify functionality and the other monthly actions to this GHA.

I have a couple additional thoughts/questions based on what I read in your PR and code.

  • I did noticed something on line 17 in contributors-data.js. It looks like twoMonthsAgo is set to the same date as oneMonthAgo. I might be missing something since I haven't been able to thoroughly test it, but I believe it should be - 2 and not - 1 at the end of line 17.
  • I believe also in contributors-data.js that the comment in line 229 in notifyInactiveMembers should say oneMonthAgo instead of twoMonthsAgo. The code itself looks correct for that function though since you're passing contributorsOneMonthAgo in on line 43.
  • In the first mention of << FUTURE in the PR, I wasn't sure exactly what the future part was referring to. It looks like the files and directories were already moved and deleted. Was there something else that it was referring to that will be done in the future that I missed?

If/when we can get the permissions issue resolved, I will circle back.

Updated definitions for `oneMonthAgo` and `twoMonthsAgo`, and corrected reference to "one month ago" in comment at function `notifyInactiveMembers`
@t-will-gillis
Copy link
Member Author

Hey @LRenDO! Thank you for your thorough review and comments.

  • Regarding the 404 response: I did not run into this when I was testing. Since I believe we have similar roles on the website teams, I am wondering if the problem could be due to our respective tokens having different scopes/ permissions? For comparison, my token scopes are: ‘admin:org’, ’project’, ’repo’, and ‘workflow’.
  • Regarding lines 15-17 in contributors-data.js: Thank you for flagging this. Although this code does work as intended (i.e., oneMonthAgo and twoMonthsAgo are actually set to the correct dates one month and two months in the past), it is confusing. Please see the revision.
  • Regarding line 229 in contributors-data.js: Good catch! Yes, the comment should say “oneMonthAgo”
  • I edited the description referring to the "<< FUTURE " revisions.

Let me know, and I would be happy to demo this if it would be helpful-

@LRenDO
Copy link
Member

LRenDO commented Aug 25, 2023

Hey @t-will-gillis!

On Lines 15-17, I was reading through too quickly. I see now that today was also being set to one month ago which made it update correctly in the code. What you've got now is definitely clearer though. The PR and comment look great too! Thanks for taking the time to review and clarify what I was seeing!

On testing, the permissions seem to cover those areas. I'll connect with you on Slack and maybe you can catch something I'm missing.

Copy link
Member

@LRenDO LRenDO 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 was able to figure out that a classic PAT is required for this GHA. I had been using a Fine-grained PAT. The testing went as expected and the changes all look great here!

I see after the issue is merged, a wiki page will be created for the GHA. If the notification implementation will not be completed in the very near future, I would mention there and/or in the test logs that the notification feature has not yet been implemented and that the logs are providing a list of folks who will be notified when the implementation is complete. Additionally, in the wiki I would make sure to include that it does not currently remove 60 day inactive members from child teams or the website team, only specifically the website-write team.

Thanks for taking the time to contribute to fixing this GHA! The GHAs can definitely be pretty cumbersome!

@LRenDO
Copy link
Member

LRenDO commented Aug 25, 2023

Hi @one2code and @yujioshiro!
I thought I would pass along how I was able to run this GHA successfully in case it's helpful for y'all.

@yujioshiro
Copy link
Member

I will start to work on reviewing this PR tomorrow. Review ETA: 9/1/2023

@t-will-gillis
Copy link
Member Author

Hi @yujioshiro would you please provide an update on your progress and let us know if you are still looking at this? Also, please ask if you would like something specific explained. This is a time sensitive issue and needs to be resolved so that other issues can proceed. Thank you

@t-will-gillis t-will-gillis merged commit dd85f71 into hackforla:gh-pages Sep 5, 2023
4 checks passed
@t-will-gillis t-will-gillis deleted the fix-bug-sch-thursday-inactive-4768 branch September 5, 2023 03:16
@roslynwythe roslynwythe mentioned this pull request Mar 15, 2024
20 tasks
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: Large 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 time sensitive Needs to be worked on by a particular timeframe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix bug that prevents GHA "Schedule Thursday 1100" from running
5 participants