Conversation
regis-underdog
left a comment
regis-underdog
left a comment
There was a problem hiding this comment.
I like using the Labels.DEPLOY model everwhere, even in the comments, but no worries. Looks good to me.
|
i think i need to make the old label still work too like i did with greyhound... gonna think more on it |
Even more reason to make the comments more generic. |
regis-underdog
left a comment
regis-underdog
left a comment
There was a problem hiding this comment.
This is significantly improved since I reviewed it.
| const isDisabled = Array.isArray(labels) | ||
| ? labels.some((item) => PrTriggerLabels.DISABLED.includes(item)) | ||
| : PrTriggerLabels.DEPLOY.includes(labels); |
There was a problem hiding this comment.
If labels is not an array, is the PrTriggerLabels.DEPLOY.includes(labels); expression valid? If labels is not an array and labels is equal to a DEPLOY label like lets-go! then isDisabled will resolve to true but that seems like it might be unexpected.
should it be PrTriggerLabels.DISABLED.includes(labels) instead?
There was a problem hiding this comment.
the enableKillSwitch function takes a labels value that defaults to an empty array...
There was a problem hiding this comment.
So what happens if your PR only has a single label and the single label is let-it-go!?
There was a problem hiding this comment.
in theory.... nothing should happen... ill test it
There was a problem hiding this comment.
So after a bit of testing the let-it-go! label doesnt make sense since it just disabling auto-deply... ive update the label to reflect that and not use lifecycle wordage
| : [ | ||
| ...new Set([ | ||
| ...currentLabels, | ||
| ...(Array.isArray(PrTriggerLabels.DEPLOY) ? PrTriggerLabels.DEPLOY : [PrTriggerLabels.DEPLOY]), |
There was a problem hiding this comment.
This seems like it would add all the deploy labels to the PR. Do we want to it to just add the first deploy label instead?
There was a problem hiding this comment.
does it matter? like just adding the first label makes it cleaner or something?
There was a problem hiding this comment.
I don't know how much it matters, but the reasons I can think of are:
- If we want people to use the first label why do we add all of them?
- Do you need to remove multiple deploy labels now to tear down your env?
There was a problem hiding this comment.
you only need to remove the initial label used
3 participants
Creates a new array constant,
PrTriggerLabelsto list labels that will trigger/disable deployments in a PR