feat(chatops-lark,tibuild): use tekton as default engine for tikv for dev-build

[wuhuizuo]

No description provided.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wuhuizuo for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[ti-chi-bot]

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

Summary:
The pull request introduces a change to use tekton as the default engine for tikv when triggering a dev build in the chatops-lark application. The approach taken involves adding a new function to determine the default engine based on the product, along with corresponding test cases. The overall quality of the changes seems good, focusing on enhancing the default behavior.

Critical Issues:
None identified in the provided diff patch.

Code Improvements:

• File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • Line 6: Import statement for slices is added but not available in the codebase. This will result in a compilation error.
    • Suggestion: Either remove the unused import or provide the correct package name if it exists.

Best Practices:

• File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • Lines 4, 152: It would be beneficial to add comments/documentation to explain the purpose of the defaultEngineForProduct function and the logic behind assigning the default engine based on the product.
  • Lines 160, 165: Similarly, adding comments to describe the purpose of the condition and the return value of the function would improve code readability and maintainability.

By addressing the identified issues and adding necessary comments, the code will be more robust and easier to understand for future maintenance.

[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 enhances the chatops-lark system by introducing an automatic default build engine for specific products. Specifically, it configures the tikv product to use tekton as its default engine when a development build is triggered without an explicit engine parameter, thereby simplifying the build command and standardizing the build process for tikv.

Highlights

• Default Engine for TiKV: The system now automatically assigns "tekton" as the default build engine for the "tikv" product if no engine is explicitly specified during a dev-build trigger.
• New Helper Function: A defaultEngineForProduct function has been introduced to encapsulate the logic for determining the default engine based on the product.
• Test Coverage: A new test case has been added to ensure the correct default engine assignment for "tikv" product builds.

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 introduces a feature to default the build engine to tekton for tikv dev-builds when no engine is specified. The changes are logical and include a corresponding test case. My feedback includes a suggestion to improve the new helper function for better readability and performance, and another to make a test case name more accurate.

[ti-chi-bot]

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

Summary:
The pull request introduces changes to use tekton as the default engine for tikv components in the dev-build process. This is achieved by adding a new function to determine the default engine based on the product and updating the logic in relevant files to use tekton for tikv non-hotfix builds. The changes include modifications in devbuild_trigger.go, devbuild_trigger_test.go, dev_build_service.go, and dev_build_service_test.go. Overall, the code changes are focused on enhancing the default engine selection for tikv dev builds.

Critical Issues:

• The import statement for slices in devbuild_trigger.go is incorrect and will cause a compilation error.
  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • This is an issue because the import statement is referencing a non-existent package.
  • Suggested Solution: Remove the incorrect import statement slices.

Code Improvements:

• The use of a hardcoded string in the defaultEngineForProduct function in devbuild_trigger.go can be improved by defining constants for the engine values.
  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • Define constants for engines to avoid hardcoded values:

    const (
        TektonEngine = "tekton"
        DefaultEngine = ""
    )
    
    func defaultEngineForProduct(s string) string {
        if slices.Contains([]string{"tikv"}, s) {
            return TektonEngine
        }
        return DefaultEngine
    }

• In dev_build_service.go, the comment explaining the engine selection logic could be improved for clarity.
  • File: tibuild/pkg/rest/service/dev_build_service.go
  • Add a more descriptive comment to explain the purpose of the engine selection logic clearly.

Best Practices:

• In devbuild_trigger.go, the defaultEngineForProduct function could benefit from a comment explaining its purpose.
  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • Add a comment above the defaultEngineForProduct function to describe its role and the logic behind determining the default engine based on the product.

By addressing the critical issue with the incorrect import statement and making the suggested code improvements, the codebase will be more maintainable and easier to understand for developers working on these components.

[ti-chi-bot]

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

Summary:
The pull request introduces changes to set Tekton as the default engine for dev-builds for TiKV components when certain conditions are met. This includes updating the logic in devbuild_trigger.go and dev_build_service.go to use Tekton as the default engine for TiKV dev-builds. The changes include adding a new function to determine the default engine based on the product and updating the tests accordingly. The overall quality of the changes is good, focusing on improving the default engine selection process for TiKV dev-builds.

Critical Issues:

• No critical issues identified in the provided diff.

Code Improvements:

• In devbuild_trigger.go, the addition of slices in the import statement seems unnecessary and could be a potential source of confusion for other developers. It's recommended to remove this import.
  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go
  • Line: 4
  • Suggested Solution:

    // Remove unnecessary import
    - "slices"

• In dev_build_service.go, when setting the default engine for TiKV dev-builds, the comment explaining the switch could be more descriptive to provide clarity on the conditions for using Tekton.
  • File: tibuild/pkg/rest/service/dev_build_service.go
  • Lines: 370-376
  • Suggested Solution:

    // Set the pipeline engine to Tekton for common dev-builds of TiKV components,
    // unless it is a hotfix build.

Best Practices:

• In dev_build_service.go, the comment for the function fillWithDefaults can be improved to provide a better explanation of its purpose and what it does.
  • File: tibuild/pkg/rest/service/dev_build_service.go
  • Lines: 370
  • Suggested Solution:

    // fillWithDefaults fills the default values for the DevBuild request, including setting the default pipeline engine based on the product and hotfix status.

Overall, the changes are well-structured and focused on improving the default engine selection process for TiKV dev-builds. The suggestions provided aim to enhance clarity, maintainability, and remove any unnecessary imports.

[ti-chi-bot]

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

Summary:
The pull request introduces changes to default the engine to "tekton" for TikV related dev-builds in both the chatops-lark and tibuild packages. This is achieved by adding a new function in devbuild_trigger.go and updating the fillWithDefaults function in dev_build_service.go. The changes include adding new tests to cover the default engine behavior. Overall, the PR aims to improve the default behavior for TikV dev-builds and is of good quality.

Critical Issues:

• No critical issues found in the provided diff.

Code Improvements:

• In chatops-lark/pkg/events/handler/devbuild_trigger.go:
  • It seems like there is an import statement for a package slices that is not being used ("slices"). This should be removed to avoid unnecessary imports.
  • Consider adding error handling for the case where the defaultEngineForProduct function returns an empty string unexpectedly. This can help in diagnosing potential issues.

Best Practices:

• In chatops-lark/pkg/events/handler/devbuild_trigger.go:
  • Add a comment above the defaultEngineForProduct function to describe its purpose, parameters, and return value.
  • Consider adding a comment above the conditional block in parseCommandDevbuildTrigger that explains the logic for setting the default engine based on the product.
• In tibuild/pkg/rest/service/dev_build_service.go:
  • Improve the comment provided for the engine switch logic to describe why tekton is chosen as the default for TikV dev-builds. This can help in understanding the reasoning behind the decision in the future.
  • Consider adding a test case to cover the scenario where req.Spec.Product is not ProductTikv to ensure the default engine logic works as expected for other products.

By addressing the code improvements and best practices, the codebase can become more maintainable, understandable, and robust.

[ti-chi-bot]

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

Summary:
The pull request introduces changes to default the engine to "tekton" for "tikv" product in the devbuild_trigger.go file and related tests, and switches to "tekton" engine for common dev-build for "tikv" components in the dev_build_service.go file. The approach involves adding a function to determine the default engine based on the product, updating tests accordingly, and modifying the default engine logic in the service file. Overall, the changes appear to be of good quality, focusing on enhancing the default engine selection based on the product.

Critical Issues:

• In chatops-lark/pkg/events/handler/devbuild_trigger.go, the import "slices" is added but not used. This can lead to unnecessary dependencies and increase binary size.
  • Suggested Solution: Remove the unused import statement.

// chatops-lark/pkg/events/handler/devbuild_trigger.go
import (
	"context"
	"flag"
	"fmt"
	// Remove the unused import below
	// "slices"
	"strings"

	"github.com/go-resty/resty/v2"
)

Code Improvements:

• In chatops-lark/pkg/events/handler/devbuild_trigger.go, the logic for setting the default engine based on the product could be extracted into a more descriptive function for clarity and reusability.
  • Suggested Solution: Extract the default engine logic into a separate function for better readability.

// chatops-lark/pkg/events/handler/devbuild_trigger.go
func parseCommandDevbuildTrigger(args []string) (*triggerParams, error) {
    // Existing code
    
    if ret.engine == "" {
        ret.engine = getDefaultEngine(ret.product)
    }

    // Existing code
}

func getDefaultEngine(product string) string {
    if product == "tikv" {
        return "tekton"
    }

    return ""
}

Best Practices:

• In tibuild/pkg/rest/service/dev_build_service.go, the use of camelCase for variable names is inconsistent. It's recommended to follow a consistent naming convention for better code readability and maintainability.
  • Suggested Solution: Use consistent camelCase naming for variables throughout the file.

// tibuild/pkg/rest/service/dev_build_service.go
// Change variable names like success_platforms, failure_platforms, triggered_platforms to succeedPlatforms, failedPlatforms, triggeredPlatforms, respectively, for consistency.

• In tibuild/pkg/rest/service/dev_build_service_test.go, there is a test case named "auto fill fips" that appears twice. Consider renaming one of them to make the test cases more descriptive.
  • Suggested Solution: Rename one of the test cases to provide a unique and descriptive name.

// tibuild/pkg/rest/service/dev_build_service_test.go
// Rename one of the "auto fill fips" test cases to make them distinct, such as "auto fill fips for tikv" and "auto fill fips for br".

[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 default "tekton" pipeline engine for the tikv product (and related components) in both chatops-lark and tibuild services for dev-build workflows. The approach adds a defaultEngineForProduct function in chatops-lark and updates the default assignment in tibuild's dev_build_service.go. It also adds relevant test coverage confirming the default behavior. The code changes are straightforward and mostly well-contained, but there are some areas for improvement in robustness and clarity.

---

Critical Issues

• Potential nil map lookup in defaultEngineForProduct (chatops-lark/pkg/events/handler/devbuild_trigger.go, lines 169-176):
  The function returns peMap[product] without checking if the key exists. If an unknown product is passed, this will return an empty string silently, which might cause unexpected behavior downstream.

  Suggestion:
  Check if the product key exists and return a sensible default or error, for example:

  func defaultEngineForProduct(product string) string {
      peMap := map[string]string{
          "tikv":          "tekton",
          "tidb-operator": "tekton",
          "tiproxy":       "tekton",
          "tici":          "tekton",
      }
      if engine, ok := peMap[product]; ok {
          return engine
      }
      return "" // or a default engine like "jenkins"
  }

  Or better, explicitly return a default engine to avoid empty strings leading to errors.

---

Code Improvements

• Refactor hardcoded engine strings and duplicated literals:
  The string "tekton" is repeated multiple times in the map and elsewhere. The tibuild code uses constants like TektonEngine. For consistency and to prevent typos, define and use constants for engine names in chatops-lark as well.

  Example:

  const (
      EngineTekton  = "tekton"
      EngineJenkins = "jenkins"
  )

  Then use EngineTekton instead of "tekton".

• Improve defaultEngineForProduct function scope and usage:
  The function is currently package-private. If it could be useful elsewhere or needs to be tested independently, consider exporting it or moving it to a shared utils package.

• fillWithDefaults logic (tibuild/pkg/rest/service/dev_build_service.go, lines 370-377):
  The logic only sets TektonEngine for tikv and non-hotfix builds. The logic is hardcoded and scattered. Consider centralizing product to default engine mapping to avoid duplication and make adding new products easier.

• Variable naming consistency in computeTektonPhase (tibuild/pkg/rest/service/dev_build_service.go, lines 524-541):
  The renaming from success_platforms to succeedPlatforms is good for Go idiomatic style, but "succeed" is a verb. Use succeededPlatforms or successfulPlatforms to keep it clear these are sets of platforms.

  Suggested rename:

  var succeededPlatforms = map[string]struct{}{}
  var failedPlatforms = map[string]struct{}{}
  var triggeredPlatforms = map[string]struct{}{}

• Tests in dev_build_service_test.go:
  The removed "auto fill fips" tests should not be deleted unless they are no longer relevant or covered elsewhere. If the feature is intentionally removed, add a comment explaining this. Otherwise, restore or adapt these tests.

---

Best Practices

• Add comments/docstrings for defaultEngineForProduct:
  The function would benefit from a comment explaining its purpose, expected inputs, and behavior when product is unknown.

• Add error handling or fallback in parseCommandDevbuildTrigger:
  When the engine is set to the default from defaultEngineForProduct, verify that it is not an empty string before proceeding, or return a meaningful error.

• Add test coverage for unknown products:
  In devbuild_trigger_test.go, add a test case where the product is not in the default engine map to confirm behavior (error or fallback).

• Consistent test formatting:
  Minor style: In devbuild_trigger_test.go, several test cases were changed to align fields vertically. While this is fine, keep consistent formatting across the file for readability.

---

Summary of suggested changes:

• Add presence check in defaultEngineForProduct before returning engine string.
• Use constants for engine names instead of hardcoded strings.
• Rename variables in computeTektonPhase for clarity (e.g., succeededPlatforms).
• Restore or explain removal of "auto fill fips" tests.
• Add comments for defaultEngineForProduct.
• Add tests for unknown product engine defaults.
• Consider centralizing default engine logic to avoid duplication.

These changes will improve robustness, maintainability, and clarity of the new default engine feature.

[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 sets the default pipeline engine for TiKV and some related products to "tekton" instead of Jenkins for dev-builds, specifically for non-hotfix builds. The approach involves modifying default engine logic in both the chatops-lark event handler and tibuild service, along with updating related tests. The overall changes are small and targeted, improving build pipeline defaults and clarifying variable naming. The code quality is generally good, but there are some improvements needed around error handling and consistency.

---

Critical Issues

• Potential panic on missing map key lookup
  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go line 170
  • Issue: The function defaultEngineForProduct returns peMap[product] directly without checking if the product key exists, which can cause a runtime panic or return an empty string silently.
  • Suggestion: Use a safe map lookup and return a default value or an error if the product is unknown, e.g.:

    if engine, ok := peMap[product]; ok {
        return engine
    }
    return ""

    Or consider returning (string, bool) to indicate presence.

---

Code Improvements

• Refactor map initialization inside function

  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go line 169
  • Issue: The map peMap is recreated on every function call, which is inefficient.
  • Suggestion: Define peMap as a package-level variable or a var initialized once, e.g.:

    var productEngineMap = map[string]string{
        "tikv": "tekton",
        "tidb-operator": "tekton",
        "tiproxy": "tekton",
        "tici": "tekton",
    }
    
    func defaultEngineForProduct(product string) string {
        return productEngineMap[product]
    }

• Improve fillWithDefaults logic clarity

  • File: tibuild/pkg/rest/service/dev_build_service.go lines 370-377
  • Issue: The nested if for setting PipelineEngine could be simplified and more extensible if more products default to Tekton later.
  • Suggestion: Consider using a helper function or map similar to defaultEngineForProduct to decide the default engine, improving maintainability.

• Consistent naming for platform sets

  • File: tibuild/pkg/rest/service/dev_build_service.go lines 528-542
  • Issue: Variable names were changed from success_platforms, failure_platforms, triggered_platforms to succeedPlatforms, failedPlatforms, and triggeredPlatforms. This is good, but consider consistently using either plural nouns or gerunds. Also, the original success_platforms may be more readable than succeedPlatforms.
  • Suggestion: Use consistent naming like successfulPlatforms, failedPlatforms, triggeredPlatforms for clarity.

---

Best Practices

• Add comments for defaultEngineForProduct behavior

  • File: chatops-lark/pkg/events/handler/devbuild_trigger.go line 168
  • Issue: The function behavior is not documented, especially the fact that it returns an empty string if no default engine is found.
  • Suggestion: Add a doc comment explaining the function's purpose and return behavior.

• Add test coverage for defaultEngineForProduct

  • File: Test files in chatops-lark/pkg/events/handler/ (none shown changed)
  • Issue: No tests verify that the default engine mapping behaves as expected.
  • Suggestion: Add unit tests covering valid products and unknown products returning empty string or error.

• Test coverage for hotfix logic

  • File: tibuild/pkg/rest/service/dev_build_service_test.go around line 106
  • Issue: The test added covers the non-hotfix Tikv case for Tekton engine. There is no test for hotfix case where Jenkins engine is expected.
  • Suggestion: Add complementary test for hotfix builds to confirm Jenkins engine remains default.

• Code style: Remove commented-out unused code

  • File: tibuild/pkg/rest/service/dev_build_service_test.go line 109
  • Issue: The commented-out CreatedBy field is unnecessary and should be removed for cleaner code.

• Consistent test descriptions

  • File: tibuild/pkg/rest/service/dev_build_service_test.go
  • Issue: The test description "auto set tekton engine for tikv non-hotfix" is clear, but previous removed tests had duplicate "auto fill fips" titles.
  • Suggestion: Ensure all test cases have unique, descriptive names.

---

Implementing the above suggestions will improve code robustness, clarity, and maintainability while avoiding potential runtime issues.