-
-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: don't fail if the tag already exists #34
base: master
Are you sure you want to change the base?
Conversation
Previously, if the tag to create was deemed to already exist, a warning would be logged and the action would not attempt to create it. However, a recently merged commit [0] changed this behaviour, by marking the action as failed if the tag already exists, instead of simply emitting a warning. Therefore, to prevent the action from failing when the tag already exists, revert to the previous behaviour and emit a warning instead. [0] ButlerLogic@8a4f4d8 Signed-off-by: Antoine Busque <[email protected]>
@coreybutler can we merge this please? |
I never saw the notice for this PR. The default behavior should not change because many workflows depend on a tag conflict to produce a failure. For example, rollback operations are often triggered with prior action failures. I'd like to see a configurable flag to provide the desired behavior for those who need an alternative. For example, an option like
|
Unless I'm mistaken, up until PR #26 was merged on October 12 2022, the default behaviour was to not fail on tag conflict, hence my PR to revert to that original behaviour. Workflows for projects I work on depended on not failing if the tags already existed. Now of course if you say the new default is to fail if it already exists and you want to keep it this way that's fine, I just want to make sure I understand correctly which ones of these is the intended default. If you want to keep the fail on tag conflict as default, I can look into adding the option to make it configurable like you suggest. I don't have a ton of bandwidth right now though so I may not be able to update the PR right away. |
I understand the confusion. PR #26 was merged because failures were happening silently. That PR made the failures noisy and introduced the dry-run concept as a way to detect a noisy failure before actually running anything. The default should fail. The override can prevent this failure. The train of thought is "the autotag action will fail if it cannot actually tag the code". The configurable option solves the problem without creating any new ones, which makes sense to me. I too am stretched very thin for time, so I'll mark this as "help wanted". @abusque - if you find yourself freeing up anytime soon and want to keep going, let me know and I'll take the "help wanted" label off. |
Understood, thanks for providing the extra context. Adding the extra option shouldn't be too complex so I'll try to find the time to do it, hopefully next week. |
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.
Hey @abusque,
If it helps, below are the changes that would implement @coreybutler's suggestions from #34 (comment)
1. Read users input from with
const failOnConflict = core.getInput('failOnConflict', { required: false }) || false;
2. Only fail if failOnConflict
is true
if (failOnConflict) {
core.setFailed(`"${tag.name}" tag already exists.` + os.EOL)
} else {
core.warning(`"${tag.name}" tag already exists.` + os.EOL)
}
3. Then in action.yml
under inputs
, add:
failOnConflict:
description: Specify if the action should fail if a tag already exists
required: false
Or if you don't have the bandwidth, I'm happy to make the updates in a fresh PR?
@Lissy93 I think it's safe to say we're all stretched for bandwidth. A fresh PR implementing this would be very welcome. If you do this, please mention this PR as well so I am sure to close it when the new one is merged. Thank you! |
Hi @Lissy93, thanks a lot for the suggested fix. I'm definitely bandwidth constrained, and the projects that prompted me to open this PR have workarounds in place already, so this item has fallen off my priority list. Definitely feel free to open the separate PR that supersedes this one. |
Previously, if the tag to create was deemed to already exist, a warning would be logged and the action would not attempt to create it.
However, a recently merged commit [0] changed this behaviour, by marking the action as failed if the tag already exists, instead of simply emitting a warning.
Therefore, to prevent the action from failing when the tag already exists, revert to the previous behaviour and emit a warning instead.
[0] 8a4f4d8
Signed-off-by: Antoine Busque [email protected]