Add labels in a more principled way. #90

csilvers · 2024-11-12T22:42:13Z

Summary:

Before, I was just munging the label to the name, so I wouldn't have
to change a lot of code. This didn't work because the name was used
in places where the code needs it to actually be a name (e.g. when
telling github what reviewers to add to the PR).

So now I store names and labels separately, and just print the label
explicitly when emitting the comment text. This also gives me more
control over where the label goes; all in all a more maintainable
arrangement.

Issue: https://khanacademy.atlassian.net/browse/FEI-5970

Test plan:

yarn jest

Before, I was just munging the label to the name, so I wouldn't have to change a lot of code. This didn't work because the name was used in places where the code needs it to actually be a name (e.g. when telling github what reviewers to add to the PR). So now I store names and labels separately, and just print the label explicitly when emitting the comment text. This also gives me more control over where the label goes; all in all a more maintainable arrangement. Issue: https://khanacademy.atlassian.net/browse/FEI-5970 Test plan: yarn jest

khan-actions-bot · 2024-11-12T22:42:32Z

Gerald

Required Reviewers

@Khan/github-actions for changes to dist/index.js, setup-files/NOTIFIED, setup-files/REVIEWERS, src/runOnPullRequest.js, src/runOnPush.js, src/utils.js, src/__test__/main.test.js, src/__test__/utils.test.js

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

lillialexis · 2024-11-12T23:05:58Z

src/runOnPullRequest.js

@@ -22,6 +22,8 @@ import {
    MATCH_COMMENT_HEADER_REGEX,
 } from './constants';

+type NameToLabelToFiles = {[name: string]: {[label: string]: string[], ...}, ...};


Before, if there were multiple matching rules, I think it would do:

{ lilli: [fileFromR1, fileFromR2, fileFromR2] ... }

Then in the comment it would do:

lilli because fileFromR1, fileFromR2, fileFromR2

But now if there are multiple matching rules, I think I would want:

lilli (rule1): fileFromR1 lilli (rule2): fileFromR2, fileFromR2

I'm not clear if this structure gets us to that.

Yes, it does! You can see in some of the test output.

Maybe it does, but the syntax is starting to twist my head...

If js allowed non-string map keys, I could do a map from (person, label) to [files], but it doesn't, so this is the best I could do...

lillialexis · 2024-11-12T23:07:24Z

src/runOnPush.js

-) => {
-    const names: string[] = Object.keys(peopleToFiles);
-    if (peopleToFiles && names.length) {
+type NameToLabelToFiles = {[name: string]: {[label: string]: string[], ...}, ...};


Can we put this somewhere shared so as to not dupe?

If you want. I like having it in each place so it's easier to find when reading the file; flow won't let them get out of sync. But I know some people feel very militant about DRY!

lillialexis · 2024-11-12T23:12:36Z

src/utils.js

-        let body = `<details>\n<summary><b>${header}</b></summary>\n\n`;
-        names.forEach((person: string) => {
-            const files = peopleToFiles[person];
+    const names: string[] = Object.keys(peopleToLabelToFiles);


I bet this impacts the #remove-me functionality. Have you checked that? This is big enough that it's fine to do that part in another PR if it's easier.

I did check that, at least by reading the code. My changes do not affect #remove-me as far as I can tell. The remove-me logic goes through getFilteredLists(), and that works on the key to peopleToLabelToFiles, which is still the person's name. (My previous way of doing this did break remove-me, but this way should not.)

Why did you think this would impact remove-me?

Why did you think this would impact remove-me?

I guess I thought it might impact the remove-me function because I didn't really know how to remove me function works… I just had a hunch that it might, and I wanted to make sure that it was considered and tested.

Sounds like the way that the functionality works is that it just re-creates the comment minus the removed person? So if the removed person was originally in there twice, and they wanted to be removed, it just leave them out for both rules? Instead of, say, matching the name in the body of the previous comment.

That's right.

lillialexis

Tentatively approving but we will want to check the #remove-me functionality.

You can also just test things in the Gerald repo, I think, if you want to avoid the long chain of gerald → actions → webapp.

There's also some local tester utility, but I have no idea what it does or how it works. I just saw it when picking though the code.

lillialexis · 2024-11-12T23:15:30Z

Here's Gerald's comment: #90 (comment) for more context on self-testing and #remove-me

csilvers · 2024-11-12T23:22:18Z

You can also just test things in the Gerald repo, I think, if you want to avoid the long chain of gerald → actions → webapp.

Sadly, I can't, because the gerald repo's github action is Khan/actions@gerald-pr-v3, so it doesn't see the new code until I go through all the rigamarole.

lillialexis · 2024-11-12T23:38:29Z

@csilvers I believe you can temporarily point Gerald to the sha that is one behind of the tip of your PR? Because the sha of the commit that points to one behind the tip is the tip...

csilvers · 2024-11-12T23:45:00Z

@csilvers I believe you can temporarily point Gerald to the sha that is one behind of the tip of your PR? Because the sha of the commit that points to one behind the tip is the tip...

Oh, I can change the git tag. But that will change things for everyone, not just me, so it's a little scary. I'll try it later in the day.

lillialexis · 2024-11-13T00:02:48Z

@csilvers I think on this branch, point this repo's workflow file to a sha on this branch. I think that should work in a way that doesn't affect other people?

csilvers · 2024-11-13T00:08:21Z

@csilvers I think on this branch, point this repo's workflow file to a sha on this branch. I think that should work in a way that doesn't affect other people?

I don't know how to do that. Right now the repo's gerald-pr.yml file looks like this:

    steps:
      - uses: Khan/actions@gerald-pr-v3
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          admin-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}

what are you proposing I change it to?

csilvers · 2024-11-13T00:44:22Z

@MiguelCastillo you're just being added because I needed a name for testing this new functionality.

There's one more test you could help with -- could you add a comment to this PR that just says #removeme? I want to test that this then auto-removes you as a reviewer.

The way I implemented it before didn't work well with REVIEWERS. Now it's a bit more complicated but also more principled. Issue: https://khanacademy.atlassian.net/browse/FEI-5970 Test plan: The comments and such on Khan/gerald#90

🖍 _This is an audit!_ 🖍 ## Summary: The way I implemented it before didn't work well with REVIEWERS. Now it's a bit more complicated but also more principled. Issue: https://khanacademy.atlassian.net/browse/FEI-5970 ## Test plan: The comments and such on Khan/gerald#90 Author: csilvers Auditors: #github-actions, jeresig, lillialexis Required Reviewers: Approved By: Checks: ⌛ Lints, ⌛ gerald Pull Request URL: #96

MiguelCastillo · 2024-11-13T21:37:44Z

@MiguelCastillo you're just being added because I needed a name for testing this new functionality.

There's one more test you could help with -- could you add a comment to this PR that just says #removeme? I want to test that this then auto-removes you as a reviewer.

Sure thang!

#removeme

MiguelCastillo · 2024-11-13T21:40:20Z

Not sure how important it is, but the tricky thing about using # to start a trigger word is that github thinks you want to reference a PR. Just an annoying observation :D

csilvers · 2024-11-13T21:42:11Z

@MiguelCastillo : I think you may need to say #removeme here, at the top level.

MiguelCastillo · 2024-11-13T21:48:16Z

@MiguelCastillo : I think you may need to say #removeme here, at the top level.

I did above. I also did it as an inline comment in case that made a difference. Will do a standalone one too.

#90 (comment)

MiguelCastillo · 2024-11-13T21:48:22Z

#removeme

csilvers · 2024-11-13T21:50:14Z

Oh, it's still processing the PR, maybe it will be a while before it's ready to run the github actions to do the update.

lillialexis · 2024-11-14T01:33:23Z

@csilvers I think on this branch, point this repo's workflow file to a sha on this branch. I think that should work in a way that doesn't affect other people?

I don't know how to do that. Right now the repo's gerald-pr.yml file looks like this:
    steps:
      - uses: Khan/actions@gerald-pr-v3
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          admin-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}
what are you proposing I change it to?

@csilvers I tried and failed :-( #91

csilvers · 2024-11-14T04:10:07Z

It's still processing the PR. Something is very wrong. Probably the huge index.js file.

csilvers · 2024-11-14T18:15:16Z

OK, I'm landing this. I'm not convinced #removeme works, but I have no experience with it, so I can't say for sure, or if not if it's this PR that broke it or not. If it's being problematic in a way that matters I'm sure we'll hear about it!

csilvers self-assigned this Nov 12, 2024

csilvers requested a review from lillialexis November 12, 2024 22:42

khan-actions-bot requested a review from a team November 12, 2024 22:42

rebuild dist file

10570dc

lillialexis reviewed Nov 12, 2024

View reviewed changes

share a type

24a71c0

khan-actions-bot requested a review from a team November 12, 2024 23:13

lillialexis approved these changes Nov 12, 2024

View reviewed changes

csilvers force-pushed the fix-reviewers branch from 84fecd9 to 24a71c0 Compare November 12, 2024 23:21

khan-actions-bot requested a review from MiguelCastillo November 13, 2024 00:39

csilvers mentioned this pull request Nov 13, 2024

[🔥AUDIT🔥] Fix the label functionality. Khan/actions#96

Merged

update setup-files with documentation

2b22ed6

khan-actions-bot removed the request for review from MiguelCastillo November 14, 2024 17:49

csilvers merged commit c9b7069 into main Nov 14, 2024
4 checks passed

csilvers deleted the fix-reviewers branch November 14, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels in a more principled way. #90

Add labels in a more principled way. #90

csilvers commented Nov 12, 2024

khan-actions-bot commented Nov 12, 2024 •

edited

Loading

lillialexis Nov 12, 2024 •

edited

Loading

csilvers Nov 12, 2024

lillialexis Nov 12, 2024

csilvers Nov 12, 2024

lillialexis Nov 12, 2024

csilvers Nov 12, 2024

lillialexis Nov 12, 2024

csilvers Nov 12, 2024

lillialexis Nov 12, 2024 •

edited

Loading

csilvers Nov 12, 2024

MiguelCastillo Nov 13, 2024

lillialexis left a comment

lillialexis commented Nov 12, 2024

csilvers commented Nov 12, 2024

lillialexis commented Nov 12, 2024

csilvers commented Nov 12, 2024 •

edited

Loading

lillialexis commented Nov 13, 2024

csilvers commented Nov 13, 2024

csilvers commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024

csilvers commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024 •

edited

Loading

MiguelCastillo commented Nov 13, 2024

csilvers commented Nov 13, 2024

lillialexis commented Nov 14, 2024

csilvers commented Nov 14, 2024

csilvers commented Nov 14, 2024

Add labels in a more principled way. #90

Add labels in a more principled way. #90

Conversation

csilvers commented Nov 12, 2024

Summary:

Test plan:

khan-actions-bot commented Nov 12, 2024 • edited Loading

Gerald

lillialexis Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lillialexis Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lillialexis left a comment

Choose a reason for hiding this comment

lillialexis commented Nov 12, 2024

csilvers commented Nov 12, 2024

lillialexis commented Nov 12, 2024

csilvers commented Nov 12, 2024 • edited Loading

lillialexis commented Nov 13, 2024

csilvers commented Nov 13, 2024

csilvers commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024

csilvers commented Nov 13, 2024

MiguelCastillo commented Nov 13, 2024 • edited Loading

MiguelCastillo commented Nov 13, 2024

csilvers commented Nov 13, 2024

lillialexis commented Nov 14, 2024

csilvers commented Nov 14, 2024

csilvers commented Nov 14, 2024

khan-actions-bot commented Nov 12, 2024 •

edited

Loading

lillialexis Nov 12, 2024 •

edited

Loading

lillialexis Nov 12, 2024 •

edited

Loading

csilvers commented Nov 12, 2024 •

edited

Loading

MiguelCastillo commented Nov 13, 2024 •

edited

Loading