fix(tibuild-v2): reject commits already tagged with tidbx-style tag

[wuhuizuo]

No description provided.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary
This PR introduces a validation to reject commits that already have an existing tidbx-style tag before creating a new tag. The approach modifies the computeNewTagNameForTidbx method to check all existing tags for one pointing to the target commit, and returns an error if found. The changes are well integrated with added logging, updated function signatures, and comprehensive new tests covering the error scenario. Overall, it is a well-structured and focused fix that improves correctness and safety of the tag creation process.

---

Critical Issues

• No critical bugs found. The implementation correctly rejects commits with existing tags, and tests cover this behavior well.

---

Code Improvements

• Improve performance by avoiding unnecessary tag pagination after an early match:
  In computeNewTagNameForTidbx (hotfix_tidbx.go, lines ~80-115), the method paginates through all tags even after detecting a tidbx-style tag on the commit. Since this is a failure scenario, it would be more efficient to immediately return the error once a matching tag is found to reduce unnecessary API calls.

  Suggested change:

  for {
      tags, resp, err := s.client.Repositories.ListTags(ctx, owner, repo, opts)
      if err != nil {
          return "", err
      }
      allTags = append(allTags, tags...)
  
      for _, tag := range tags {
          name := tag.GetName()
          if pattern.MatchString(name) && tag.Commit != nil && tag.Commit.SHA != nil && *tag.Commit.SHA == commitSHA {
              return "", &hotfix.HTTPError{
                  Code:    http.StatusBadRequest,
                  Message: fmt.Sprintf("commit %s already has existing tidbx-style tag: %s", commitSHA, name),
              }
          }
      }
  
      if resp.NextPage == 0 {
          break
      }
      opts.Page = resp.NextPage
  }

• Consider extracting the tidbx-style tag matching logic into a helper function:
  The regex match and commit SHA comparison logic appears in the tag name parsing loop. Extracting this into a helper method would improve readability and facilitate future reuse or testing.

---

Best Practices

• Add comments about the new commit SHA parameter:
  The addition of the commitSHA argument to computeNewTagNameForTidbx changes its contract. A comment explicitly describing the purpose of this parameter (to detect existing tags on that commit) near the function signature would improve maintainability.

• Logging consistency:
  The newly added warnings and errors are helpful. Consider adding the repository and commit context to all log messages for clearer tracing, e.g.,

  l.Warn().Str("repo", p.Repo).Msg("Invalid repository format...")

• Test coverage looks good but can be expanded:

  • There is a positive test for the error case when a commit is already tagged, which is excellent.
  • It would be helpful to also add a test case ensuring the happy path still works when a commit is not tagged, to verify no regressions.

---

Minor Notes

• In the test TestComputeNewTagNameForTidbx (hotfix_tidbx_test.go), variable tagedCommitSHA should be renamed to taggedCommitSHA for spelling consistency.

• The log messages in BumpTagForTidbx have been improved for clarity, which is good.

---

Overall, this PR is a solid enhancement that improves correctness by preventing duplicate tagging of commits. Addressing the suggested performance optimization and adding a bit more documentation would raise the quality further.

[gemini-code-assist]

Summary of Changes

Hello @wuhuizuo, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a crucial fix to the tibuild-v2 hotfix tagging service by preventing the creation of duplicate 'tidbx-style' tags for the same commit. It enhances the service's robustness by adding explicit checks for already-tagged commits and improves operational clarity through more detailed logging and a refined tag message. The changes are backed by comprehensive new test cases to ensure reliability.

Highlights

• Prevent Duplicate Tagging: A new validation check has been implemented to prevent the creation of a new 'tidbx-style' tag on a commit that has already been tagged with such a tag. This ensures that each commit has a unique 'tidbx-style' tag, avoiding redundancy and potential confusion.
• Enhanced Logging: Warning logs have been added for common validation failures, such as invalid repository format or missing branch/commit information. Error logs are also now present for failures during commit verification, tag name computation, and tag creation, significantly improving the debuggability and observability of the hotfix tagging process.
• Improved Tag Message Clarity: The message used when creating a hotfix tag has been updated from 'Hot fix tag created by %s' to 'Created hot fix tag on behalf of %s'. This change provides clearer context regarding who initiated the tag creation.
• Comprehensive Testing: New test cases have been added to thoroughly cover the scenario where a commit is already tagged with a 'tidbx-style' tag. This includes both unit tests for the tag computation logic and an end-to-end test for the BumpTagForTidbx service, ensuring the new rejection logic functions correctly.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly implements a check to prevent creating a new tidbx-style tag on a commit that already has one. The changes are well-structured, including improved logging and comprehensive tests for the new logic. My review includes a couple of suggestions for improvement: one to optimize a loop by removing a redundant regular expression match, and another to make a test condition more robust to better match the implementation's logic.

[Copilot]

Pull request overview

This PR enhances the BumpTagForTidbx functionality to prevent creating duplicate tidbx-style tags on the same commit. The implementation adds validation to reject commits that already have an existing tidbx-style tag (matching the pattern vX.Y.Z-nextgen.YYYYMM.N), returning a descriptive error to the caller.

• Added commit SHA validation to detect and reject already-tagged commits
• Enhanced logging throughout the tag creation workflow
• Updated the tag creation message format for clarity
• Comprehensive test coverage for the new validation logic

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
experiments/tibuild-v2/internal/service/impl/hotfix_tidbx.go	Implements commit validation logic to reject already-tagged commits, adds structured logging at key points, and updates the tag message format
experiments/tibuild-v2/internal/service/impl/hotfix_tidbx_test.go	Adds test cases for the commit-already-tagged scenario in both unit and integration test contexts

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary
This PR adds a validation step in the BumpTagForTidbx service to reject commits that already have an existing tidbx-style tag. The approach modifies computeNewTagNameForTidbx to accept a commit SHA and scan all tags, returning an error if a tidbx-style tag already points to that commit. It also updates the main flow and adds relevant tests to cover this new behavior. Overall, the implementation is solid, integrates cleanly, and includes good test coverage for the new logic.

---

Critical Issues

• Test Code Typo:
  • File: hotfix_tidbx_test.go, line ~95
  • Issue: In TestComputeNewTagNameForTidbx, the field tagedCommitSHA is misspelled (should be taggedCommitSHA).
  • Why: This could cause confusion or accidental bugs if the field is referenced elsewhere.
  • Suggestion: Rename tagedCommitSHA → taggedCommitSHA consistently.

---

Code Improvements

• Log Context Consistency

  • File: hotfix_tidbx.go, lines 24, 34, 45, 63
  • Issue: Logs like l.Warn().Msg(...) and l.Err(err).Msg(...) are used but the logger l is not explicitly defined in the snippet shown. It's assumed to be part of the receiver or context but not clear.
  • Why: To avoid confusion and ensure logs have proper context.
  • Suggestion: Confirm l is initialized properly before use or add l := s.logger or similar at the start of the function.

• Repeated Pattern Compilation

  • File: hotfix_tidbx.go, line 93 (inside computeNewTagNameForTidbx)
  • Issue: The regex pattern pattern is used but not shown how/where it’s defined. If it's compiled every time the function runs, it may be inefficient.
  • Why: Regex compilation can be costly if done repeatedly.
  • Suggestion: Ensure the pattern is compiled once at package initialization, e.g.

    var tidbxTagPattern = regexp.MustCompile(`some_regex`)

    and reused in the function.

• Error Message Clarity

  • File: hotfix_tidbx.go, line 109
  • Issue: The error message returned when a commit already has a tidbx-style tag is clear but could also include guidance on what to do next.
  • Suggestion: Consider adding a hint, e.g.

    Message: fmt.Sprintf("commit %s already has existing tidbx-style tag: %s; please use a different commit or remove existing tag", commitSHA, name),

• Test Case Coverage Suggestion

  • File: hotfix_tidbx_test.go
  • Issue: The new test TestBumpTagForTidbx_FailWhenCommitAlreadyTagged covers the rejection scenario well but only for a tidbx-style tag on the first page.
  • Suggestion: Add an additional test case where the tidbx-style tag is on a later page to ensure pagination is handled correctly.

---

Best Practices

• Function Documentation Update

  • File: hotfix_tidbx.go, lines 70-73
  • Issue: The comment for computeNewTagNameForTidbx was updated to mention the commit check but could be more explicit about the new parameter and behavior.
  • Suggestion: Update comment to:

    // computeNewTagNameForTidbx computes the next tag name based on existing tags,
    // and returns an error if the specified commit already has a tidbx-style tag.
    // Parameters:
    // - ctx: context for cancellation and deadlines
    // - owner, repo: repository information
    // - commitSHA: commit to check for existing tags
    // Returns the new tag name or an error.

• Test Assertions Consistency

  • File: hotfix_tidbx_test.go, line ~130
  • Issue: The test uses t.Fatalf for error checking but does not always check error contents/assert error messages.
  • Suggestion: Add assertions on the error message string to ensure the error is as expected, not just the error type and code.

• Import Cleanup

  • File: hotfix_tidbx_test.go, line 7
  • Issue: The "strings" package is imported after tests were added; ensure all imports are used and sorted.
  • Suggestion: Run goimports or equivalent to clean and sort imports.

---

Overall, the PR effectively adds the required validation with good test coverage and clean integration. Addressing the minor issues above will improve maintainability, clarity, and robustness.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary
This PR enhances the BumpTagForTidbx function to reject commits that are already tagged with a tidbx-style tag. The approach involves modifying computeNewTagNameForTidbx to accept a commit SHA and error out if any existing tidbx tag already points to this commit. The PR also adds comprehensive tests to verify this new behavior. Overall, the changes are well-structured, maintainable, and improve correctness by preventing duplicate tagging.

---

Critical Issues

• None found. The logic correctly checks existing tags against the commit SHA to prevent duplicate tagging, and the error handling is consistent.

---

Code Improvements

• Improve error message consistency and logging context
  • File: hotfix_tidbx.go lines ~45, ~55, ~65
  • Why: Some log messages are generic ("Failed to verify commit") without context like repository or commit SHA, which would help debugging.
  • Suggestion: Enrich logs with repository and commit info for better traceability, e.g.:

    l.Err(err).Str("repo", p.Repo).Str("commit", commitSHA).Msg("Failed to verify commit")

• Avoid repeated calls to tag.Commit != nil && tag.Commit.SHA != nil
  • File: hotfix_tidbx.go line ~105
  • Why: This check is repeated inside the loop for every tag.
  • Suggestion: Extract this to a variable for clarity:

    if pattern.MatchString(name) && tag.Commit != nil && tag.Commit.SHA != nil && *tag.Commit.SHA == commitSHA {
        // ...
    }

• Consider extracting tag pattern regex to a package-level constant
  • File: hotfix_tidbx.go
  • Why: The pattern is used multiple times and may benefit from a single definition accessible by other parts of the package or tests.
  • Suggestion: Define var tidbxTagPattern = regexp.MustCompile(...) at the package level.

---

Best Practices

• Add PR description and method-level comments

  • File: Entire PR, especially BumpTagForTidbx
  • Why: The PR description is empty, and some new functions (especially the modified BumpTagForTidbx) lack detailed comments about the new commit-tag check feature. This would improve maintainability and onboarding.
  • Suggestion: Add a short PR description and expand comments describing the reason for rejecting commits already tagged.

• Improve test coverage for edge cases

  • File: hotfix_tidbx_test.go lines ~270
  • Why: The new test verifies rejection of already tagged commits, but it would be good to also test:
    • A commit with no tidbx-style tags passes successfully.
    • Tags that partially match the pattern but are not exactly tidbx-style do not cause rejection.
  • Suggestion: Add small unit tests for these cases to strengthen coverage.

• Style: consistent comment formatting

  • File: hotfix_tidbx.go lines ~70-80
  • Why: Some comments start with lowercase ("Tags follow the pattern...") and others with uppercase.
  • Suggestion: Follow Go doc conventions: comments should start with the function name and be full sentences. For example:

    // computeNewTagNameForTidbx computes the next tag name based on existing tags,
    // and fails if the provided commit already has a tidbx-style tag.

• Naming: clarify parameter names

  • File: computeNewTagNameForTidbx signature
  • Why: Passing commitSHA as the last argument is fine, but naming it commitSHA explicitly in the signature improves clarity.
  • Suggestion: Consider named parameters or document in the comment.

---

Overall, this PR adds a valuable correctness check with appropriate tests and logging. The suggestions above would improve observability, maintainability, and robustness.

[wuhuizuo]

/approve

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wuhuizuo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment