Hi @tbulva! This MR just got merged by @PatrickRice. There is no associated issue / work item for it but the MR description hopefully provides a good enough rationale.

@heidi.berry, @PatrickRice: MR updated. Thank you for sharing the embarrassment; it's making it more bearable. 😅

Christoffer Sawicki (f89164ef) at 17 Mar 07:09

Oh, yes, of course! This is embarrassing. I'll fix this up tomorrow morning.

Above reviews are now out of date after MR update.

@PatrickRice: I have now updated the MR hoping I understood you correctly. Please have a look!

Christoffer Sawicki (4efea3df) at 15 Mar 14:37

... and 16 more commits

@PatrickRice: Yes, that's correct. For me personally it would be better to log it immediately and unconditionally but I feared that there are users that rely on the unprotect + reprotect branch who would not appreciate the extra logging. I didn't extend the reprotect error handling code because the message "Unable to protect repository tag" makes sense when you're trying to protect a tag (unlike "Unable to unprotect repository tag"). I don't have any strong preferences on how and what we log as long as we don't swallow the error that tells the user what's going on. If you have a preference I'm happy to adjust the change (or let you do it if you so prefer).

No worries; I didn’t follow the recommendation. 🙂 Thanks for merging❣️

I asked @GitLabDuo for a review using chat as the automatic review failed (see !2999 (comment 3148356798)). Here's what it said:

Summary

This is a small, well-motivated community contribution by @qerub that fixes error handling in the Create method of resource_gitlab_tag_protection.go. The existing code reuses the err variable when calling UnprotectRepositoryTags, which shadows the original ProtectRepositoryTags error and makes it impossible to understand why the initial protection attempt failed. The fix is correct and the change is minimal (1 file, ~6 lines).

What the change does

1. Introduces a separate unprotectErr variable instead of reusing err, so the original protection error is preserved.
2. Improves the error message when unprotection also fails, now reporting both errors: "Unable to protect repository tag (%s) and also unable to unprotect it (%s)".
3. Adds parenthetical formatting to the reprotect error message for consistency.

Feedback

Positive:

• The bug is real and the fix is correct. The original err from ProtectRepositoryTags was being silently discarded, making failures very hard to diagnose.
• The combined error message is a clear improvement for troubleshooting.
• Minimal, focused change with no unrelated modifications.

Suggestions:

1. Consider using fmt.Errorf wrapping or errors.Join for a more idiomatic Go error composition, though the current fmt.Sprintf approach is consistent with the rest of the codebase, so this is minor.
2. The underlying "unprotect + reprotect" logic itself is fragile (as the author acknowledges). If the initial ProtectRepositoryTags fails for a reason other than "tag already protected" (e.g., permissions, network error), the code still attempts to unprotect, which could remove an existing protection unintentionally. A future improvement could check the error type before attempting the unprotect. However, this is out of scope for this MR and the author explicitly calls this out.
3. The pipeline is currently failing - it would be worth checking whether the failure is related to this change or is a pre-existing/infrastructure issue.

Verdict: This is a clean, correct fix. I'd recommend approving it after confirming the pipeline failure is unrelated.

1 Error: bc46137b: Use full URLs instead of short references (gitlab-org/gitlab#123 or !123), as short references are displayed as plain text outside of GitLab. For more information, take a look at our Commit message guidelines.

I can take care of this but will note that the merge commit messages of this project (from where I picked the reference) also break this rule.

...in order to make it easier to troubleshoot why tag protection creation fails.

The current unprotect + reprotect logic is IMHO undesirable but not something we can change (as somebody might depend on it) but improving the logging is a step in the right direction. I'm currently having an issue with creation failing with "Unable to unprotect repository tag: 404 Not Found" which was completely impossible to understand until I read the code.

Christoffer Sawicki (f3a6fb53) at 10 Mar 22:26

Christoffer Sawicki (ce846f08) at 10 Mar 22:25

... and 1 more commit

The documentation for gitlab_tag_protection states that one should set create_before_destroy = true to avoid the risk of ending up without tag protection if resource replacement fails.

This is a bad idea because because there can only be one tag protection per tag. Typically, the new replacement resource would fail to create due to conflict but due to the resource's automatic unprotection code it will succeed only for it to then be destroyed silently (leaving no tag protection).

Note: This recommendation was added in 9b7f761b / !1549 but I was not able to find a rationale. Let me know if my reasoning is incorrect.

Christoffer Sawicki (bc58f997) at 10 Mar 21:53

Fixup to 23d0f8dd added with !2964.

Ideally this should have been detected in the CI/CD pipeline. Let me know if you agree.

Christoffer Sawicki (bc46137b) at 10 Mar 21:36

Christoffer Sawicki (7069f792) at 10 Mar 19:49

Christoffer Sawicki (1c2437f5) at 10 Mar 19:47