labeler icon indicating copy to clipboard operation
labeler copied to clipboard

Published 20 hours ago • verified publisher icon actions

Fix 'sync-labels' setting

Open epuertat opened this issue 4 years ago • 8 comments

Closes https://github.com/actions/labeler/issues/104

Following syntax at https://github.com/actions/toolkit/blob/master/packages/glob/README.md#recommended-action-inputs

epuertat avatar Nov 18 '20 16:11 epuertat

Thanks for the patch, @epuertat! Labeler was behaving incorrectly and your changes corrected that behavior. I took the liberty of updating to the current state of main branch, and adding tests.

What this means is we're in the weird position where Doing The Right Thing means a breaking change, since labeler will now behave differently with the same inputs out there in the wild. A load-bearing bug.

IMO this necessitates a v4 release and a big callout in the changelog.

Before

Before this change: if you provide literally any value for sync-labels, labeler would Sync Labels. (In particular, if you had sync-labels: false, labeler would actually Sync Labels. Yikes.)

After

After this change: you must provide true to get Sync Labels behavior. Providing false will NOT Sync Labels. (And any other value will be treated like false.)

pje avatar Jun 21 '21 23:06 pje

Thanks for taking care of the tests, @pje!

epuertat avatar Jun 22 '21 08:06 epuertat

@pje Any ETA on merging and deploying a new version? We are considering removing the action in SciPy because the sync mechanism is annoying us. I really like the idea of this action and would like to keep it 😃

tupui avatar Oct 15 '21 09:10 tupui

Hey. Can you please merge/release this?

wknapik avatar Dec 09 '21 22:12 wknapik

Anything we can do to help move this forward and get this merged?

hashhar avatar Feb 09 '22 10:02 hashhar

I am sad to say that due to this, we (SciPy) dropped this actions. Hope that this get solved at some point so we can reconsider.

tupui avatar Feb 09 '22 16:02 tupui

@tupui the workaround for this is fairly easy, you just need to set sync-labels to an empty string:

sync-labels: ""

Obviously, this should be fixed but no need to drop the action when there's workaround available :P

Jackenmen avatar Feb 09 '22 16:02 Jackenmen

@tupui the workaround for this is fairly easy, you just need to set sync-labels to an empty string:

sync-labels: ""

Obviously, this should be fixed but no need to drop the action when there's workaround available :P

Oh thanks! We sadly dropped it a few months ago already because there was no action there... I just got notified about this with the recent message and thought I would comment again to say we had to take action–as I feared we might need to.

A "hack" might not make our maintainer team confident I am afraid. I will keep an eye on this.

tupui avatar Feb 09 '22 16:02 tupui

Hi @pje,

This is a small gentle up: is there anything to be done for this fix to be merged?

Major libraries' CI (like SciPy's and scikit-learn's) would largely benefit from this fix. :pray:

jjerphan avatar Nov 11 '22 08:11 jjerphan

Hello @epuertat! Thank you for the contribution and sorry for the late review! I left a comment, please take a look. Also, please rebuild dist folder and resolve conflicts.

MaksimZhukov avatar Jan 10 '23 10:01 MaksimZhukov

@epuertat: Thank you for this contribution, I am looking forward to it being integrated. Are you still working on it? If not, can I supersede it, @MaksimZhukov? :slightly_smiling_face:

jjerphan avatar Jan 30 '23 07:01 jjerphan

@jjerphan @MaksimZhukov @adam-azarchs: please feel free to take over and move forward with this.

epuertat avatar Jan 30 '23 09:01 epuertat

Hello @epuertat ! Thank you for the response! I am closing the PR in favor of this one. @jjerphan please find the details in that PR.

MaksimZhukov avatar Jan 30 '23 12:01 MaksimZhukov