fix: Limit number of labels added to 100 #497

markmssd · 2023-02-02T21:37:33Z

Description:
Github Issues can have at most 100 labels assigned to them. This PR limits the labels being added to 100, and logs the rest as a warning.

Closes #561
Closes #562

~~TODO: Unit tests, will add them if the PR makes sense!~~

Related issue:
Add link to the related issue.

Check list:

Mark if documentation changes are required.
Mark if tests were added or updated to cover the changes.

macksal · 2023-03-08T06:53:50Z

dist/index.js

            for (const [label, globs] of labelGlobs.entries()) {
                core.debug(`processing ${label}`);
                if (checkGlobs(changedFiles, globs)) {
-                    labels.push(label);
+                    if (labels.length >= GITHUB_MAX_LABELS) {


I think this neglects the number of labels that are already present. Say the PR has 50 labels, and we come up with 100 that we want to apply, but only 25 of those are already present. The correct result would be to add 50 new ones, 25 remain the same, and 25 would be excess. But the current code reports 100 labels with no excess.

Good point. I've moved the addLabels to happen AFTER the removeLabels (which happens with syncLabels enabled only) to mitigate this issue. But maybe there's a better way still, I'll check again. Thanks!

To me, an easy solution seems to be first getting the labels already on the issue with listLabelsOnIssue. Then, you have all the information needed to ensure the total count remains less than 100 while tracking accurately all additions/removals that you want to do.

#### Summary GitHub does not support more than 100 labels per PR, so the PR labeler action fails when that happens. There's ongoing work to fix it, see actions/labeler#497, but I think we can ignore the action failures for now. <!--

markmssd · 2023-03-22T00:04:04Z

@macksal Alright so I ended up simply using pullRequest.labels, which already holds all the PR's labels. Then:

I hydrate the labels array with those current labels only if syncLabels=false
Push all matching labels into the labels array (making sure there is no duplicates)
Splice the array at 100 (the maximum number of labels allowed on a PR). This splits the original labels array to a length of 100, and all the rest is returned into excessLabels array.
If syncLabels=true, I use setLabels, otherwise I addLabels. Either way, it's guaranteed to never set/add more than 100 labels
If there was an excess in excessLabels array, simply log them

If that makes sense I'll add unit tests!

markmssd · 2023-04-18T17:41:08Z

@macksal I've rebased my branch to resolve conflicts. Kindly let me know if it needs any more changes (other than unit tests)!

dusan-trickovic · 2023-05-31T16:21:45Z

Hello, @markmssd ! Thanks for creating the PR - we love and endorse customer contributions! Overall, your PR looks good, it definitely deals well with the scenario of trying to add more than 100 labels and with what to do with the excess.

However, we would also like you to take a look at this PR and its changes. Maybe it can help you see if anything could be added and/or refactored. Furthermore, those two together could solve this issue, as the latter deals with reducing the array by filtering duplicates, but not limiting the number of labels to 100, which former takes into account.

In addition to that, there's another finding that could prove to be beneficial to the solution as well and which we strongly recommend incorporating into the final application. Namely, upon some inspection on our end, we have discovered that the Action will likely keep throwing HttpError: Server Error unless we split the total number of labels in the array past the certain index and send them in parts instead. That 'golden number', as far as our tests go, turns out to be 48.

What that means is that if there are less than 48 labels present in the array, those can be sent all at once without any problems. As soon as that number is exceeded, the array should be split. We suggest splitting it in half, as this seems to be the most efficient way.

However, keep in mind that, as the number of labels keep increasing (e.g. 100 labels), splitting it in half could also give us more than 48 labels to work with. As a solution to this, you could put some kind of a loop in place that could perform the action above for as long as needed (e.g. 100 gets split into 2x50, which then split to 2x25).

Example of this part of the logic:

const LABELS_LIMIT_TO_ADD_AT_ONCE = 48;

while (labelsToBeAdded.length > 0) {
    let splicedLabelsList: string[] = [];
    splicedLabelsList = labelsToBeAdded.splice(0, LABELS_LIMIT_TO_ADD_AT_ONCE / 2);
    await sendLabels(client, prNumber, splicedLabelsList);
}

I hope I explained this clearly :) For any further clarifications, questions and suggestions, please feel free to reach out to us again and we will try to answer them as best as we can. And, of course, feel free to also add some unit tests :)

Thank you for your time and contribution to our software :)

dusan-trickovic · 2023-06-06T08:17:22Z

Hello, @markmssd ! I just wanted to give you a little ping to see if there are any new updates on your end? Maybe some thoughts about my previous suggestion? Thank you in advance :)

markmssd · 2023-06-07T02:52:11Z

Hey @dusan-trickovic , thanks for the thorough explanation!

I just got back from vacation and I'll need a few days before getting back to this PR.

So if I understand correctly, this HTTP Server Error happens only when using the "addLabels" method, or also when using "setLabels"? If that's the case, I could also change the PR to ONLY use setLabels, and it would fix all issues.

The logic would be:

if syncLabels=true, use setLabels with new labels only.
if syncLabels=false, use setLabels with current PR's labels + new ones

(In both cases it would dedupe labels and trim to 100 max)

However if the HTTP Server Error happens with setLabels too, then I'll go with your suggestion with the golden number 48, what do you think?

src/labeler.ts

markmssd · 2023-06-08T02:00:49Z

@dusan-trickovic everything is done and tested, let me know what you think and if it requires some changes!

dusan-trickovic

Hello @markmssd ! Thank you again for creating this PR! I have written a few initial review points that should be addressed before continuing on. Please let us know if you have any additional questions about them, any additional suggestions or concerns :)

src/labeler.ts

MaksimZhukov

Hello @markmssd! Thank you for the contribution!
Looks good to me! I left a minor comment

src/labeler.ts

MaksimZhukov · 2023-06-19T11:12:06Z

@markmssd could you please resolve conflicts?

dsame · 2023-06-19T17:55:13Z

.gitignore

+lib/
+.vscode/
+.idea/


Till the moment the IDE settings were avoided to be added to the repo

Oh I can remove them haha, it was just getting annoying locally

dsame · 2023-06-19T18:22:14Z

src/labeler.ts

@@ -39,23 +43,33 @@ export async function run() {
      configPath
    );

-    const labels: string[] = [];
-    const labelsToRemove: string[] = [];
+    const labels: string[] = pullRequest.labels.map(label => label.name);


May be it worth to create Set object at this point and avoid !labels.includes(label) check further

I initially tried with a Set but the code was getting really ugly, I'll try that again 🤔

Alright I pushed changes that use Sets instead, let me know what you think

markmssd · 2023-06-20T00:40:22Z

Conflicts have been served ✅ I also added an additional check to only call setLabels if there was actually a change, to avoid unnecessary API calls, and fixed the tests accordingly.

dusan-trickovic · 2023-06-21T09:45:32Z

Hello @markmssd ! The PR looks good, we will be merging it now. Thank you very much for your contribution! :)

push to excess labels to avoid reaching the limit

756f0bd

markmssd requested a review from a team as a code owner February 2, 2023 21:37

markmssd changed the title ~~Limit number of labels added to 100~~ fix: Limit number of labels added to 100 Feb 2, 2023

build dist

e4e6956

macksal reviewed Mar 8, 2023

View reviewed changes

erezrokah mentioned this pull request Mar 8, 2023

chore(ci): Ignore PR labeler errors cloudquery/cloudquery#8816

Merged

markmssd added 2 commits March 21, 2023 19:37

never set more than 100 labels

7429fbb

use splice instead of set

e173de7

Merge branch 'main' into fix/max-100-labels

56b1a22

markmssd added 4 commits June 7, 2023 20:07

Merge branch 'main' into fix/max-100-labels

f90e3c3

ignore IDE folders

0e55d23

install @octokit/plugin-retry

4beee0f

always setLabels

267e6ba

markmssd commented Jun 8, 2023

View reviewed changes

src/labeler.ts Show resolved Hide resolved

markmssd added 3 commits June 7, 2023 21:11

fix indentations

b35f25a

fix specs

5baeff5

add spec for excess labels

d65a48d

markmssd added 2 commits June 12, 2023 08:34

prettier

4dd2b81

licensed cache

2f9caa0

dusan-trickovic suggested changes Jun 12, 2023

View reviewed changes

src/labeler.ts Outdated Show resolved Hide resolved

src/labeler.ts Show resolved Hide resolved

src/labeler.ts Outdated Show resolved Hide resolved

src/labeler.ts Show resolved Hide resolved

markmssd added 3 commits June 12, 2023 23:52

revert to !!core.getInput('sync-labels')

112b189

better warning for exceeded labels

3d7acab

keep manually-added labels

92a11d8

nest the dedupe logic

ffc8ca7

MaksimZhukov reviewed Jun 15, 2023

View reviewed changes

src/labeler.ts Outdated Show resolved Hide resolved

rename removeLabel to removeLabelFromList to avoid confusion

3c2f134

MaksimZhukov approved these changes Jun 15, 2023

View reviewed changes

dmitry-shibanov approved these changes Jun 15, 2023

View reviewed changes

dusan-trickovic approved these changes Jun 15, 2023

View reviewed changes

marko-zivic-93 approved these changes Jun 16, 2023

View reviewed changes

dsame approved these changes Jun 19, 2023

View reviewed changes

unrealclouder added 4 commits June 19, 2023 19:48

Merge branch 'main' into fix/max-100-labels

66f07b8

use Sets, and issue a call only if labels have actually changed

287ce90

remove IDE config folders from gitignore

05a4b4b

remove obsolete duplucation check

c9ee1b4

MaksimZhukov approved these changes Jun 20, 2023

View reviewed changes

marko-zivic-93 approved these changes Jun 20, 2023

View reviewed changes

IvanZosimov approved these changes Jun 21, 2023

View reviewed changes

dusan-trickovic merged commit 7a202e6 into actions:main Jun 21, 2023

markmssd mentioned this pull request Jul 5, 2023

Fix Labeler Issue #561 - HttpError Issue akv-platform/labeler#1

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Limit number of labels added to 100 #497

fix: Limit number of labels added to 100 #497

markmssd commented Feb 2, 2023 •

edited

Loading

macksal Mar 8, 2023

markmssd Mar 20, 2023

macksal Mar 21, 2023

markmssd commented Mar 22, 2023

markmssd commented Apr 18, 2023

dusan-trickovic commented May 31, 2023

dusan-trickovic commented Jun 6, 2023

markmssd commented Jun 7, 2023 •

edited

Loading

markmssd commented Jun 8, 2023

dusan-trickovic left a comment

MaksimZhukov left a comment

MaksimZhukov commented Jun 19, 2023

dsame Jun 19, 2023

markmssd Jun 19, 2023

dsame Jun 19, 2023

markmssd Jun 19, 2023

markmssd Jun 20, 2023

markmssd commented Jun 20, 2023

dusan-trickovic commented Jun 21, 2023

fix: Limit number of labels added to 100 #497

fix: Limit number of labels added to 100 #497

Conversation

markmssd commented Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmssd commented Mar 22, 2023

markmssd commented Apr 18, 2023

dusan-trickovic commented May 31, 2023

dusan-trickovic commented Jun 6, 2023

markmssd commented Jun 7, 2023 • edited Loading

markmssd commented Jun 8, 2023

dusan-trickovic left a comment

Choose a reason for hiding this comment

MaksimZhukov left a comment

Choose a reason for hiding this comment

MaksimZhukov commented Jun 19, 2023

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

markmssd commented Jun 20, 2023

dusan-trickovic commented Jun 21, 2023

markmssd commented Feb 2, 2023 •

edited

Loading

markmssd commented Jun 7, 2023 •

edited

Loading