Enhanced Skills Activity GHA to use Skills Directory

[xnealcarson]

Fixes #8316

What changes did you make?

• added most recent skillsIssueNums.csv to the github-actions/utils/_data folder, and converted the directory to JSON format (skills-directory.json).
• Created a new module in utils that looks up in the directory and return the information (skills-directory.js).
• added import module to post-to-skills.js for the skills directory
• Refactored code in post-to-skills-issue.js to search the newly added skills directory before deferring to the querySkillsIssue() GitHub API, if necessary, upon which new info found using querySkillsIssue() will be saved to the skills directory
• Tested changes by creating fake skills issue on my repo's project board (Skills Issue: Developer: Xavier Neal-Carson (Test) xnealcarson/website#17) and then creating subsequent test issue to ensure proper function of Skills Activity tracking (Test Issue for Refactored Skills Activity GHA xnealcarson/website#18)

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

• We needed to reduce the number of unnecessary GitHub API calls that the "Member Activity Trigger" workflow makes when posting to a user's Skills Issue by providing a user directory. New info found

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.

Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown

Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.

• I have checked this PR for CodeQL alerts and none were found.
• I found CodeQL alert(s), and (select one):
  • I have resolved the CodeQL alert(s) as noted
  • I believe the CodeQL alert(s) is a false positive (Merge Team will evaluate)
  • I have followed the Instructions below, but I am still stuck (Merge Team will evaluate)

Instructions for resolving CodeQL alerts

If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

In general, CodeQL alerts should be resolved prior to PR reviews and merging

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

No visual changes to the website

NOTE TO REVIEWERS

If you would like to test these changes for yourself:

• Make sure your HACKFORLA_GRAPHQL_TOKEN is active
  • Change the default branch of your repo to a test branch
  • Change line in activity.trigger.yml to refer to your own repo.
  • Add a Skills Issue in your name to your project and assign it to yourself. Take note of the issue number and add it below.
  • The automation uses GraphQL queries to locate each user's Skills Issue and the bot comment. HfLA's project, status, and field ids are hard-coded in the file github-actions/utils/_data/status-field-ids.js, so you will need to tell the bot the number of your repo's Skills Issue by making the following edits:
    • In post-to-skills-issue.js, change:

              const isActiveMember = await checkTeamMembership(github, context, eventActor, TEAM);
      

      to:

              // const isActiveMember = await checkTeamMembership(github, context, eventActor, TEAM);
              const isActiveMember = true;
      

    • Change lines 39-44 in post-to-skills-issue.js to:

        // Get eventActor's Skills Issue number, nodeId, current statusId (all null if no Skills Issue found)
        // const skillsInfo = await querySkillsIssue(github, context, username, SKILLS_LABEL);
       const skillsIssueNum = _the number of the skills issue you created_ ;  
       const skillsIssueNodeId = null;
       const skillsStatusId = null;
       const isArchived = false;
      

[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 xnealcarson-enhance-gha-skillsactivity-8316 gh-pages
git pull https://github.com/xnealcarson/website.git enhance-gha-skillsactivity-8316

[ryanfkeller]

Hi @xnealcarson, I'll give this a review as well! ETA 10/17, generally able to be available 1-5p weekdays.

[ryanfkeller]

Hey @xnealcarson, thanks for taking this issue! It looks like you're on the right track, and also great job on the PR description -- it made it easy to understand what you've done here.

I noticed a couple things in my review:

• In post-to-skills.js, I think you have some old code on line 55 that creates a syntax error on trying to re-define skillsInfo (and if it ran would result in an unneeded re-query for the skills issue). That line can probably just be deleted. Nitpick but the extra indentation on the following lines could probably also be removed.
• In post-to-skills.js, I can see that we are using a query to search for the "activity comment" even when we got the issue number from the local JSON. Since I believe the JSON includes the comment ID under the "id" category, you should be able to just read/patch that comment directly and skip the search if there is a JSON entry. The query/search you have should definitely still be a fallback for when you don't have the issue info saved locally.
• Since we do save the "activity comment" id in the skills-directory.json, we'll want to make sure to update that JSON with the ID when we have to create a new "activity comment".
• We may not want to save the skillsIssueNums-*.csv in the _data directory, since it isn't used directly and with this flow in action, I expect the .json should be the source of truth.
• (Edited, I missed some things in your description in my original comment, sorry!) Instead of changing the post-to-skills-issue.js to bypass isActiveMember and skillsInfo collection, would it be possible to instead modify our local copy of the _data sources so the full script flow is tested? We probably also want the test case of the skills issue not existing in _data as well so the query is executed. If you have examples of that, could you share the workflow run link?

Again, really nice work! Looking forward to seeing the next rev 🚀

[ryanfkeller]

Looking good, but a few things I think need attention! See my comment above.

[xnealcarson]

Thanks for the check-in @DDVVPP. I just picked up last night where I left off, after getting a bit sidetracked this past week. I really let time get ahead of me.

I'm about wrapped up with requested changes, save for a couple things, so I am not quite ready to send out those re-requests. I will check in again with another status update (hopefully the last!) tomorrow after 3pm PST. So sorry for the delay and thank your for your patience @ryanfkeller and @t-will-gillis.

[xnealcarson]

Ran into some issues working on the last of each of your requested changes, which I Slack'd @t-will-gillis about. He aims to assist me with those tomorrow, so I will give another follow-up then once I've received his feedback!

[t-will-gillis]

Hey @xnealcarson Thanks and sorry for mixing up the function names. As I mentioned in Slack, the file I meant is utils/skills-directory.js and the function is lookupSkillsDirectory(). What I was meaning to say is that from what I can tell, return directory[eventActor] is returning a null and that you would need to lookup the directory info a different way- using find() (I said .filter() but find is more appropriate).

I threw many console.log() s into the function to show what is happening:

function lookupSkillsDirectory(eventActor) {
    const directory = loadDirectory();
    console.log(``);
    console.log(`At lookupSkillsDirectory() in skills-directory.js :`);
    console.log(`eventActor: ${eventActor}`);
    console.log(`directory[eventActor]: ${directory[eventActor]}`);
    console.log(``);
    
    const result = directory.find(entry => entry.eventActor === eventActor);
    if (result) {
      console.log(`result: ${JSON.stringify(result)}`);
      console.log(`result["issueNum"]: ${result["issueNum"]}`); // just to verify
    }
    console.log(``);
    console.log(`return 'result' rather than 'directory[eventActor]`);
    console.log(``);
    
    // return directory[eventActor] || null;
    return result || null;

If you sub these temporary log messages in, I think result is what you are looking for, and something similar would also be true for updateSkillsDirectory().

Thanks again for sticking with this!

[xnealcarson]

Hey @ryanfkeller and @t-will-gillis . My apologies for the delay in implementing your requested changes. I've just added Will's suggested changes for utils/skills-directory.js and am ready to re-request your reviews for all the changes that I have made. If there are any additonal edits that need to be made outside of these initial changes, I will be sure to deliver them more promptly in an effort to close out this issue before the approaching Thanksgiving and December breaks. Many thanks for your time and patience. I will be on the lookout for your second reviews!

[ryanfkeller]

Hi @xnealcarson, hopefully my inline comments in this review don't turn into a formatting mess! All my comments (and this) are IMO.

Overall I think this is a great update. I agree with all the changes you made re: my initial review -- especially nice work on the logic with the cached comment ID. I think that the logical flow of your code is solid and accomplishes the goal nicely (except for one off-nominal case where the comment at the cached comment ID doesn't have the marker).

There are a few placed where I think there are small coding errors, where maybe some stuff got accidentally deleted. I called out everything I saw, but it might be good to try re-running the tests once you solve them to see if you get good results.

There are also a couple places with nitpicks (like tab formatting & limiting console comments) that I'd like to see updated before merging.

Great work, thanks for sharing! Let me know if I missed anything or if you have any questions.

[ryanfkeller]

It might be good to clean up some of these console.logs to retain only what would be useful to someone looking at the logs. Since I think this will be called a lot AND we expect to regularly get both results existing and not existing, it might be better to not have prints here and let the caller code handle printing.

[ryanfkeller]

Is this supposed to be commentIdCached? This seems to be serving the purpose of commentIdCached, which is not initilized when it is used below.

[ryanfkeller]

Question: are we good to end the function here, or do we still want to hit the block at the end where we potentially re-open the skills issue? If we want to hit that block, we'll need to remove the return.

[ryanfkeller]

I'm still interested in your thoughts on this one as well. My guess is that we would still want to hit the final if statement to potentially reopen the skills issue.

[ryanfkeller]

If we don't satisfy this if statement, I think we accidentally skip the fallback (because commendIdToUse is set to commendIdCached, which is non-null if we get here)

[ryanfkeller]

I think this comment still applies. You could potentially resolve this by adding an else that sets commentIdToUse to null.

[ryanfkeller]

Nitpick but would be nice to have this indent on this comment match the code block

[t-will-gillis]

Hi @xnealcarson echoing what @ryanfkeller said- great work!

Here are some comments in addition to Ryan's:

• I may have mislead you with the code I left in my last comment. I was using temporary console.log()s to help with troubleshooting lookupSkillsDirectory(), but as Ryan notes it will be good to clean up / remove the logs for the commit.
• Previously, the first line in lookupSkillsDirectory() was: const directory = loadDirectory();. If you add this back it addresses Ryan's comment that loadDirectory() is not being used, as well as the comment that directory is not initialized.
• (need const directory in updateSkillsDirectory() also?)
• As Ryan notes- it looks like lookupSkillsDirectory() is missing the closing bracket. (This would clear up one of the CodeQL alerts also)
• Agree with Ryan's note that commentFoundId should be commentIdCached
• I think that the updateSkillsDirectory() might need some work still

Thanks for working on this! Getting closer

[ryanfkeller]

Hey @xnealcarson, in response to your comment, I think this CodeQL scan is about result, not directory. This function doesn't use the result that is collected. That makes sense to me, because this function is basically overwriting the entry that would have been found for result, right?

It seems to me like you can just delete the result line.

[xnealcarson]

Hey @ryanfkeller and @t-will-gillis ! Sorry for the delay and many thanks for the detailed feedback. I did find your inline comments to be very helpful @ryanfkeller, along with your supporting comments @t-will-gillis.

We seem to be getting closer to wrapping up indeed with the changes I've made. As I'm writing this comment though, I see the CodeQL alert about the unused result variable in updateSkillsDirectory, which I believe has been triggered by my addition of the const directory at the top of that respective function, per your tentative suggestion in your latest comment @t-will-gillis . Should I not have added const directory in updateSkillsDirectory` then?

Other than that though, I will be on the lookout for any further suggestions you may have. Thanks again to both of your for your patience and help with this issue!!

[ryanfkeller]

Hey @xnealcarson, again, nice work! You knocked out most of my comments, and I marked those as resolved. There are two that I think still apply -- I added sub-comments and left them open. Finally, I think the last CodeQL warning is accurate and I left a comment on that.

Once those are set, I'd be really interested in seeing a last test run using your test infra just to make sure it's all still working the way we expect.

This is looking super close! Thanks for sharing!

[ryanfkeller]

I think this comment still applies. You could potentially resolve this by adding an else that sets commentIdToUse to null.

[ryanfkeller]

I'm still interested in your thoughts on this one as well. My guess is that we would still want to hit the final if statement to potentially reopen the skills issue.

[t-will-gillis]

Hey @xnealcarson Here is a heads up before we talk on Zoom:

• The skills-directory.json file that I gave you has the wrong kind of issue ids. My bad of course. Two solutions: one is to clear the file completely and let your function fill in the values as needed, and the second is for me to generate a new file. (FYI I discovered that GitHub elements such as issues use different ids at different times. I can explain the reason if you want, but to keep it short, the ids currently shown in the JSON file will cause an error later in this function.)
• In post-to-skills-issue.js, I think that the call to updateSkillsDirectory() will need to happen later in the file, after finding the id of the comment with the MARKER, and not in the if (!skillsInfo) loop. Something you could do is trigger a flag in the loop to run the update after the MARKER comment is found.
• Also, the parameters for updateSkillsDirectory() need to match up with the four values in the skills-directory.json.
• Finally, I previously mentioned that the updated skills-directory.json needs to be committed so that the changes are actually saved. There is an action by "stefanweifel" that we use in a few places for this reason. Check out schedule-monthly.yml line 42 onward- this will let us save the updated JSON.

Again, thank you for keeping on this