Skip to content
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

feat: support tidb dashboard for devbuild #2721

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Conversation

lijie0123
Copy link
Contributor

@lijie0123 lijie0123 commented Jan 5, 2024

Why:

  • devbuild trigger tidb-dashboard
  • tidb-dashboard suport build environment var

Signed-off-by: lijie <lijie@pingcap.com>
@ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo January 5, 2024 03:33
@ti-chi-bot ti-chi-bot bot added the size/L label Jan 5, 2024
Copy link

ti-chi-bot bot commented Jan 5, 2024

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

Summary of changes

  • The pull request aims to add support for FIPS (Federal Information Processing Standards) to the TiDB dashboard.
  • Additional parameters IsDevbuild, BinaryPrefix, DockerImg, BuildEnv are introduced in Jenkins pipeline for controlling the build process.
  • The docker images are built with different parameters depending on whether it's a development build or a release build.
  • The build process for different architectures (amd64, arm64, darwin/amd64, darwin/arm64) is specified in the Jenkins pipeline.
  • SHA256 checksum is generated for each build artifact.
  • The upload process for different build artifacts is updated according to the new changes.
  • The building process of tidb-dashboard is moved into a sub-pipeline when the product is tidb-dashboard.

Potential issues

  • There is a lack of descriptive comments in the pull request. This can make it harder for other developers to understand the changes.
  • There are many changes in the pipeline structure which might affect existing builds.
  • There seems to be no explicit check or configuration to ensure FIPS compliance. The title suggests FIPS support, but the changes don't seem to directly related to FIPS.
  • There are no updates in test cases or no new test cases added to verify the new changes.

Suggestions

  • Add descriptive comments to the pull request to explain the changes in detail, especially regarding how these changes relate to FIPS support.
  • Test the new pipeline thoroughly to ensure it doesn't break existing builds.
  • If the pull request is meant to add FIPS support, make sure to include explicit checks or configurations for FIPS compliance.
  • Update or add new test cases to cover the new changes.

@lijie0123 lijie0123 changed the title feat: support fips for tidb dashboard feat: support tidb dashboard for devbuild Jan 5, 2024
Signed-off-by: lijie <lijie@pingcap.com>
Copy link

ti-chi-bot bot commented Jan 5, 2024

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

Review Summary

This pull request mainly includes the modifications of five files to support the tidb_dashboard for a dev-build. It allows for more flexible configuration and adds a new feature for a dev-build.

Key Changes

  • Adds multiple new build parameters to build_tidb_dashboard.groovy, including IsDevbuild, BinaryPrefix, DockerImag, and BuildEnv.
  • In build-tidb-dashboard.groovy, it introduces more parameters, replaces hard-coded values with these parameters, and adds sha256 checksum calculation for the built binaries.
  • Adds new rules in dev-build.groovy to determine whether to build with subpipeline or multi-arch build based on the Product parameter.
  • Changes the file names that are used when downloading and publishing the tidb-dashboard binaries in publish-tidb-dashboard.groovy.

Potential Problems

  1. There can be a typo in the parameter name DockerImag in dev-build.groovy. It's likely it should be DockerImg to align with other files.
  2. The code doesn't handle the possible empty states for the parameters DockerImg, BinaryPrefix, and BuildEnv. It would be better to add default values or error handling in case these parameters are not provided.
  3. The IsDevbuild parameter is used to determine whether to run certain stages but it's not clear how it's set.

Suggestions

  1. Correct the typo in the parameter name DockerImag.
  2. Add default values or error handling for the parameters DockerImg, BinaryPrefix, and BuildEnv.
  3. Make sure the IsDevbuild is correctly set and used as per the requirements of the build process.

Signed-off-by: lijie <lijie@pingcap.com>
Copy link

ti-chi-bot bot commented Jan 5, 2024

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

Summary of Changes

This pull request introduces support for the TiDB Dashboard in the development build. The following are the key changes:

  1. New parameters have been added to the pipeline job build-tidb-dashboard in jenkins/jobs/cd/build_tidb_dashboard.groovy.

  2. The dockerfile variable has been changed from final to def type in jenkins/pipelines/cd/build-tidb-dashboard.groovy. This allows for the insertion of build environment variables.

  3. The BinaryPrefix, DockerImg and BuildEnv parameters are now used to configure the docker image name, binary file path, and build environment.

  4. Additional changes are made to the pipeline stages to use these new parameters.

  5. Changes are made to the dev-build.groovy and publish-tidb-dashboard.groovy files to support the new parameters and build process.

Potential Problems

  1. There is a lack of error handling and validation for the new parameters. This could lead to unexpected behaviour if incorrect values are passed.

  2. The pull request does not include any tests to validate the new functionality.

  3. The dev-build.groovy file has changes that separate the build process for tidb-dashboard from other products, this may increase the complexity and maintenance of the code.

Suggestions for Improvement

  1. Add error handling and validation for the new parameters to ensure they meet expected formats and values.

  2. Include tests to verify the new functionality works as expected and does not introduce regressions.

  3. Consider refactoring the changes in dev-build.groovy to reduce code complexity and improve maintainability.

Review Markdown

### Code Review for Pull Request # (insert PR number here)

#### Summary of Changes

This pull request introduces support for the TiDB Dashboard in the development build. The key changes involve the addition of new parameters to configure the build process and changes to the pipeline stages to use these parameters.

#### Potential Problems

1. There is a lack of error handling and validation for the new parameters. This could lead to unexpected behaviour if incorrect values are passed.
2. The pull request does not include any tests to validate the new functionality.
3. The `dev-build.groovy` file has changes that separate the build process for `tidb-dashboard` from other products, this may increase the complexity and maintenance of the code.

#### Suggestions for Improvement

1. Add error handling and validation for the new parameters to ensure they meet expected formats and values.
2. Include tests to verify the new functionality works as expected and does not introduce regressions.
3. Consider refactoring the changes in `dev-build.groovy` to reduce code complexity and improve maintainability.

Copy link
Collaborator

@purelind purelind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@ti-chi-bot ti-chi-bot bot added the lgtm label Jan 5, 2024
Copy link

ti-chi-bot bot commented Jan 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: purelind

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

Copy link

ti-chi-bot bot commented Jan 5, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-05 06:02:26.255946963 +0000 UTC m=+2409637.293173890: ☑️ agreed by purelind.

@ti-chi-bot ti-chi-bot bot added the approved label Jan 5, 2024
@ti-chi-bot ti-chi-bot bot merged commit c613ea3 into main Jan 5, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the tidb-dashboard-fips branch January 5, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants