-
Notifications
You must be signed in to change notification settings - Fork 90
Conversation
@brendanator Thanks for this PR! It would be nice if we could also exclude prereleases not yet included in a release. Is that possible? Ex.: latest release is 0.9.6, I've got a couple of 0.9.7-beta.x prereleases that will be in a future 0.9.7. I would like all prereleases excluding the 0.9.7-beta.x to be deleted. |
No, there's no understanding of release ordering. The way I've been using it to do something similar is to combine it with the arg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @brendanator I think most users expected a regex for delete_tag_pattern
since it was named 'pattern'. See for example #10 which your PR might be a way to fix this assumption. I think forcing this to be a regex expression is fine and do not think a separate flag is needed to control if using regex or not as in PR #19.
Do you feel the same way as I do and that we can just merge your PR as is?
Or do you feel a regex flag as was done in PR #19 would be more appropriate and you want to make changes to your PR here?
I agree with your reasoning @thadguidry and would be happy for this PR to be merged as is 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to improve with some checking.
if (deletePattern) { | ||
console.log(`releases containing ${deletePattern} will be targeted`); | ||
let deletePattern = new RegExp(process.env.INPUT_DELETE_TAG_PATTERN || ""); | ||
if (process.env.INPUT_DELETE_TAG_PATTERN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendanator Don't you need to have an empty check within this if() ? What happens if deletePattern
just before cannot be created (is empty or fails to be assigned as a valid variable... then how could the if() even evaluate that env as a boolean... it would be a regex pattern from the previous let
assignment, not a boolean, right? Look at the above previous code lines to see how const
variable assignment was used to do boolean checks and console logging. Perhaps we can do something similar for deletePattern
?
Think through this a bit please and improve a bit here the potential failures, then we can merge.
@smallprogram Any thoughts on this PR? I had a few requests from others to deal with prereleases, as well as improve delete_tag_pattern but I myself have not tested this in production. Take your time, we got all year 😉 |
let me think about it |
this pr can close @thadguidry |
delete_tag_pattern
to be a regex