Consolidate functions in schedule-monthly.yml 5969

[t-will-gillis]

Fixes #5969

What changes did you make?

• Added inactive-members.md markdown template to provide a more easily edited and maintainable template for the GHA schedule-monthly.yml.
• Added issue-template-parser.js to the /utils folder to read and parse the template.
• In create-new-issue.js, removed the hard-coded data for creating a new issue and replaced with a call to the template parser.
• In contributors-data.js, addressed a bug whereby assignees were incorrectly considered as active whenever the bot commented. Fix is for activity to be counted only when the assignee causes the activity (for example, by commenting, etc.)
• Also in contributors-data.js, removed fs.readFile() at previous lines 287-291. This function's only purpose was to confirm that the fs.writeFile() was successful- it is no longer required.
• Also in contributors-data.js, addressed the requirement that team members with open issues are not removed from the "website-write" team: 60-70 team members currently are 'inactive' but are blocked from removal from the team b/c of open issues: the log file now lists these inactive members and the open issues they are assigned to. UPDATED: At dev meeting on 1/15/24, it was discussed that member activity should not consider the open "Pre-work Checklist" issues. Refer to Line 135.
• Also at the 1/15/24 meeting, we discussed how to account for a dev such as 'cnk' that is not active and is also not a 'Maintainer' but still should retain access. It was decided that members of the 'website-maintain' should not be removed. There are a couple of ways to accomplish this, but it seems the easiest is to define the 'website-maintain' team as permanent contributors. In contributors-data.js, refer to lines 13, 74-75, and 142-144.

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

• To make files easier to edit, reuse, and maintain as noted above.

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

No visual changes.
GHA logs in order to show reporting for inactive members who are blocked from team removal due to open issues.
Inactive Team Member issue created using the inactive-members.md template.

Notes to PR Reviewers

• You will need to have a functioning test environment on your local repo. In addition to the 'files changed' in the PR, there are additional changes that you should make to help with testing. If you do not make these edits, you might delete or mis-edit Hack for LA data, delete team members, generate false or junk notifications, and/or create junk issues on HfLA's live project board.
• In the schedule-monthly.yml file:
  • Replace HfLA tokens with a personal token***, 4 locations. (*** or name your token 'HACKFORLA_BOT_PA' )
  • Line 12, redirect to your personal repo.
• In the wr-schedule-monthly.yml file:
  • Line 46, replace HfLA token with a personal token.
• In github-actions/trigger-schedule/github-data/contributors-data.js:
  • Disable the api call that deletes inactive users (in case a member's name slips through). Replace the current code on lines 287-293 with:

          // await octokit.request('DELETE /orgs/{org}/teams/{team_slug}/memberships/{username}', {
          //   org: org,
          //   team_slug: team,
          //   username: username,
          // })
          console.log("Would be deleted: "+ username);
          removedMembers.push(username);
        }

• Also in contributors-data.js
  • Any test runs should reset the "oneMonthAgo" and "twoMonthsAgo" to several months ago so that the test run does not accidentally remove a member outside of the normal once-a-month window. Therefore on lines 18-26, replace current code with:

let today = new Date();
let twoMonths = (today.getMonth() == 1) ? 6 : 5;           // If month is "February" == 1, then twoMonths = 3 months ago

let oneMonthAgo = new Date();                              // oneMonthAgo instantiated with date of "today"
oneMonthAgo.setMonth(oneMonthAgo.getMonth() - 4);          // then set oneMonthAgo from "today"
oneMonthAgo = oneMonthAgo.toISOString();
let twoMonthsAgo = new Date();                               // twoMonthsAgo instantiated with date of "today"
twoMonthsAgo.setMonth(twoMonthsAgo.getMonth() - twoMonths);  // then set twoMonthsAgo from "today"
twoMonthsAgo = twoMonthsAgo.toISOString();

• In github-data/github-actions/trigger-schedule/list-inactive-members/create-new-issue.js:
  • Line 15, change "owner" to your personal repo
  • Lines 37-38, add a space after the "@ " so that members id'd by the test run do not receive notifications. Thus replace current lines with:

  let removedList = removeList.map(x => "@ " + x).join("\n");  // please make sure there is a space after "@  "
  let notifiedList = notifyList.map(x => "@ " + x).join("\n");  // please make sure there is a space after "@  "

• In github-data/github-actions/trigger-schedule/list-inactive-members/comment-issue.js:

• UPDATED: In create-new-issue.js

  • Line 16, change "owner" to your personal repo can keep context.repo.owner
  • Line 18, change the agendaIssueNum to an existing issue in your personal repo

• In github-data/github-actions/trigger-schedule/list-inactive-members/inactive-members.md:

  • Line 5, add the labels to your repo (the test might create them automatically)
  • Line 6, the 'milestone' number '8' coincidentally corresponds to the "08. Team Workflow" milestone in HfLA's repo. If you don't have milestones defined yet, you can add your first one and change this number to '1', etc.

After doing the above, you can trigger schedule-monthly.yml manually. If the gha runs successfully, it will open -then immediately close- a new issue in your repo called "Review Inactive Team Members". It will also add a comment to the issue that you listed for agendaIssueNum in create-new-issue.js.

[github-actions]

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-consolidate-funct-sch-month-5969 gh-pages
git pull https://github.com/t-will-gillis/website.git consolidate-funct-sch-month-5969

---

Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:

https://github.com/t-will-gillis/website/blob/consolidate-funct-sch-month-5969/CONTRIBUTING.md  

[LRenDO]

ETA: 1/18/24 10am
Availability: 1/17/24 10am-3pm

[LRenDO]

Hi @t-will-gillis!
Great job with GHA! It looks like it's functioning well!

• The branching was done correctly
• Issue number was linked
• The PR title is descriptive of the changes
• Changes were made correctly in the code
• Manual testing shows the comment is added correctly and the new issue is created correctly
• Results were accurate for notifying and removing members when randomly verified
• Relevant logs were included
• The PR request clearly states what was updated
• Additional testing instructions were included

I have a couple non-functional requests and a few questions about the intended functionality.

• There is a missing closing bracket at the end of the file in contributors-data.js

• In line 190 of contributors-data.js the issueNum parameter can be removed as it is not being used

• In the instructions for testing, for the changes in github-actions/trigger-schedule/github-data/contributors-data.js, I believe it's lines 285-291 not 273-279 that need to be replaced (probably changed after some edits). Might be helpful to update it, if we plan to wait on another review.

• On line 308 the API call for the user's repo could probably be simplified using this request /repos/{owner}/website and catching it if it doesn't exist, rather than looping through each repo. However, I also know that I have seen some folks who didn't use website as their cloned repo name. We may want to keep it as is and check every repo to see if it's parent's full_name is hackforla/website instead of checking it by the repo variable. I will leave it to you about how you want to address this. I think it's okay as it is, but it these could be marginal improvements.

• I'm assuming the 2 months becomes 3 months ago for February because December is a break month. Should we also do the same thing for January for the one month ago that is done for February or do we not care because its just an additional message and folks aren't getting removed?

• Lines 321-330 seem at first glance to be redundant since maintainers are added to the allContributorsSince list at line 142. It looks like there is a separate role that is different from being added to the maintainer team. Is that something we should consider standardizing or is there circumstance where a user who has the maintainer role shouldn't be on the website-maintain team?

• It would likely be helpful to add the list of inactive members with open issues so they can be handled appropriately either in the agenda page, the new issue, or an entirely new issue. Is there a plan to incorporate that list in some way?

• It looks like the list to notify in the issue includes not only the people who have been inactive for a month, but also the people who have already been removed (inactive for more than 2 months). Bringing this up to make sure this is the intended behavior.

• The issue outlines a procedure for temporary leaves. Are there plans to automate checking for temporary leaves to prevent removal? Or is the plan just to add them back when they return?

Future suggestions for refactoring and issues

I took some notes on some future refactoring and easy issues that could be created and thought I would include them here to get your thoughts. As you mentioned on Slack, there is definitely still a fair amount of refactoring that can be done.

• In contributors-data.js
  • fetchContributors() and the other longer functions could be refactored
  • Line 200 I agree this appears to be redundant
• The functions in create-new-issue.js could all be good utility functions. Arguments or context could be passed into main to replace template variables.
• For calling the API in our GHAs, it looks like we are using octokit directly and actions/github-script that uses octokit indirectly and provides repo context as well. We might want to consider choosing one and sticking with it so that we can more easily refactor and create utilities. For example, github-actions\utils\getTimeline.js could be quickly swapped in for the getEventTimeline function in contributors-data.js. Right now it's possible to use it, but we would need to mimic the context object.
• The context object (context.repo.owner context.repo.repo) can be used instead of hardcoding the owner and repo variables in create-new-issue.js and comment-issue.js unless we want to move to octokit.
• A standalone good first issue could be to delete line 1 and 2 in comment-issue.js

Again, great work and thanks for taking the time to contribute and continuing to take on these GHAs!

[freaky4wrld]

ETA: Evenings
Availability: 1/20/24

[t-will-gillis]

Hey @LRenDO many, many thanks for your very thorough review and corresponding notes. Here are responses to the first part of your review- more to follow

• changed instructions to reference lines 287-293,
• in contributors-data.js:
  • line 382: added missing “}”,
  • line 191: removed issueNum and irrelevant comment, and
  • line 128: removed issueNum
  • line 308 moved to 320: yes, your suggestion is much better. Changed the API call to check the created_at for the member’s “website” repo rather than looping through all of them. Doy! Regarding the members who don’t name their repos “website”: Turns out that one of the test runs caught dozens of members without a specific “website” repo. Besides that, this check would only apply to only a few people at most- like a new member who forks the repo and then does nothing at all. We could be checking for parent.full_name === hackforla/website but I am leaning towards leaving this as is or even removing this check entirely.
  • lines 18-26: correct, the twoMonthsAgo == 3 is to account for HfLA’s December break. I did not change oneMonthAgo to 2 in February b/c I was assuming that an ‘active’ member could take December off and then start up again in January and be current. >> On the other hand, there would be little harm in counting a member’s December activity. I’ll change oneMonthAgo to match.
  • lines 309-318 and lines 143-146: This is quirky. The members of the website-maintain team are not the same as the ‘Maintainers’ on website-write, so the two sections are filtering different people (with some overlap)
  • Regarding the inactiveWithOpenIssues list: other than listing them on the action logs, we have not discussed what to do with that list. I added this check originally b/c we have 50-60 inactive members with open “Pre-work Checklists” that I wanted to highlight, but now we will remove them. What are your thoughts?

It looks like the list to notify in the issue includes not only the people who have been inactive for a month, but also the people who have already been removed (inactive for more than 2 months). Bringing this up to make sure this is the intended behavior.

• Yes- thanks for bringing up. This happens because the test run is not removing anyone from the team. On the live run, line 40 fetches the current team members that will be used for comparing and removing the two months inactive members. Then line 54 fetches the current team members a second time- now excluding the two months inactive members. This new team list is then filtered for the one month inactive members.

The issue outlines a procedure for temporary leaves. Are there plans to automate checking for temporary leaves to prevent removal? Or is the plan just to add them back when they return?

• Right now, there are no specific plans that I know. My opinion is that we add them when they come back. We can still talk more about this- but my gut feel is that there are few people that fall into this category, and that it is not that much of a hassle for someone to need to ask to be put back on the ‘website-write’ team.

OK that is all for now- I will continue in a separate comment with your suggestions for refactoring.

[freaky4wrld]

Extending the ETA: EOD 1/25/24, maybe a need to look through it a bit more and have clear understanding of the issue and GHA as well..... I hope it's not a time-sensitive issue

[t-will-gillis]

Hey @LRenDO (and @freaky4wrld )!
Thank you for the helpful comments and ideas about refactoring. Your notes about using context.repo.owner etc. is a good idea- incorporated into the latest. You mention the annoying part about how we are using the newer Octokit (2022) API in two files vs. the previous API in the others. It would be nice to refactor to use the same structure in all files. In any case, maybe the schedule-monthly should use one or the other.

To extend what you are saying, the /util/ files don't need to be passed the github and context parameters themselves, e.g. get-timeline.js's parameters could be owner and repo.

Also-

• in contributors-data.js , lines 200-215 removed redundant code checks for assignee comments as discussed above
• in create-new-issue.js:
  • substituting github.repo.owner, github.repo.repo at owner and repo per your suggestion
  • refactoring to use /util/post-issue-comment.js in place of comment-issue.js, and removed latter file
• in schedule-monthly.yml removed step for 'comment-issue'

Marking ready for re-review.

[t-will-gillis]

Hey @freaky4wrld - thanks for the update. Please feel free to message @LRenDO or me if there is anything we can help you with.

[freaky4wrld]

Hey @t-will-gillis first of all hats-off for working on such an issue, and thanks for providing testing-guidance and bear with me throughout the procedure. Here you go with my review :

• The to/from branch are correct
• The linked issue is correct as well
• The changes suggested and done by you suits well as of my understanding of the issue
• Testing done manually was successful performing automatic issue-creation, identifying the inactive members, notifying potential inactive members and posting a comment linking the issue created
• Thanks for providing additional testing guide, the reason for making the changes was clear and specifying the location where changes are done
• In contributor.js the suggestion for providing correct value to oneMonthAgo and twoMonthAgo was implemented correctly, the fetchContributors( ) function looks apt and produces correct results also the suggestion to use the format /repos/{owner}/website in API call is implemented.
• In the same filegetEventTimeline( ) maybe this a very minor change you can name the arra variable to something descriptive like timelineArray it’s upto you the logic looks fine and working correctly.
• The hard-coded variables and template is removed from create-new-issue.js and a new file with a descriptive and appropriate name inactive-member.md is created and fetched correctly to create a new issue
• The issue-template-parser.js is correctly extracting the data and the template from the inactive-member.md which is then used by create-new-issue.js, nice idea for
• The file comment-issue.js is replaced with post-comment.js which is working correctly in posting a comment and notifying the project leads about the issue created.
• Lastly I would like to say you considered most of the edge cases, the rest can be considered and fixed in the future when the actions work weirdly (which it doesn’t do currently).

I guess that’s all from my side, at first I was overwhelmed by the scope of this issue but thanks @t-will-gillis and @LRenDO for guidance and providing resources to review this…..
There’s a lot to learn from you guys, PR approved……..

[LRenDO]

Hi @t-will-gillis and @freaky4wrld!

@freaky4wrld, thanks for the kind words! As @t-will-gillis said, feel free to reach out! Always happy to help!

@t-will-gillis, thanks for clarifying all the things and great work on the updates! Everything is functioning correctly. I have one potential request to take a look at. The rest are responses and refactoring thoughts.

Potential Request:

• I might be thinking about this incorrectly. Maybe you can clarify it for me. In contributors-data.js, I believe the condition for the oneMonth variable should be 0 for January not 1 for February. As it is now, when it checks in February, oneMonthAgo causes it to look back 2 months to December. If I understood correctly, we are looking to skip December since most folks won't contribute that month. If we change it oneMonthAgo to 2 months in January, it should look back to November which I believe is the behavior we are looking to create.

Other Stuff:

• Good catch on the refactoring to use post-comment!
• Totally lost track of the order of operations. You're absolutely right the reason the folks who were supposed to be removed were on the notified list was because they weren't actually removed for testing. Thanks for pointing it out!
• It is an excellent point to change the context variables of the util functions to repo and owner. Though if there were any other parameters that need to passed it could get start to get a bit cumbersome as most of those functions already have 4 parameters. We would still need github itself because it is the octokit we use to call the API, but we could maybe name it something more clear or add a clarifying comment. From what I can see, it doesn't look like getTimeline is actually being used. I think it could be a good medium, maybe large, issue swap out getEventTimeline() in contributor-data.js for getTimeline, unless you want to take it on now.
• On the topic of having a lot of parameters in some of the workflows, I think there is some room for refactoring and creating a class or two. For example a class for an API instance and we could abstract some of the calls to the API as well as have an easier way to pass the octokit and the context. Potentially, a comment class could be useful as well. This is definitely a bigger scope than this issue though.

[t-will-gillis]

Hey @freaky4wrld and @LRenDO Thank you both for your thoughtful reviews and suggestions. Nayan- I agree about renaming arra more descriptively in getEventTimeline() and after this PR for \utils\getTimeline.js - this is renamed per your suggestion. Ren- I love the idea of creating classes for an API instance and for comments- I definitely would like to explore and discuss after this PR.

Regarding contributors-data.js and the one/two month variables: currently, schedule-montlhy.yml runs normally on December 1st, then skips/ does not run on January 1st due to the Dec. break, and then resumes on February 1st. The February run defines twoMonthsAgo as January, December (b/c the month is off), and November. A member whose last activity was on October 31st should be considered two-months-inactive, but a member whose last activity was on November 2nd should not. oneMonthAgo is defined as January and adds in December as a ‘bonus’ month. A member whose last activity was between November 1st through the 30th would be considered one-month-inactive, and a member whose last activity was between December 1st and January 31st would be considered as an active member. On March 1st the run returns to its typical behavior. That is the intent, at least. Let me know if this seems correct to you

I made another small change in schedule-monthly.yml: line 77 refactored to remove createNewIssue =; we only need to script to run and don’t need the variable.

Thanks so much again and let me know if there is anything else

[LRenDO]

@t-will-gillis, I missed the cron at the top of schedule-monthly and didn't realize it wasn't running in January. It all makes sense now. Thanks for taking the time to clarify! Yes, let's definitely chat about potentially designing a significant refactor for the GHAs.