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

fix: add target branch env var #209

Merged
merged 2 commits into from
Jan 16, 2024
Merged

fix: add target branch env var #209

merged 2 commits into from
Jan 16, 2024

Conversation

jayl1e
Copy link
Contributor

@jayl1e jayl1e commented Jan 16, 2024

Why:

fix build grafana error:

empty branch/tag info for tidb

@ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo January 16, 2024 09:26
Copy link

ti-chi-bot bot commented Jan 16, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the PR title and diff, it appears that the changes are related to adding a new environment variable TARGET to the build process that was not present before. This variable is being used to generate the release artifacts, specifically the grafana tarball.

There are no potential problems that can be identified from the diff as it is a simple addition of an environment variable. However, it is always a good practice to make sure that the new environment variable is properly documented and added to the documentation.

Suggestions:

  • It is always a good practice to document new environment variables that are added to the build process. It is recommended to add this new variable to the documentation.
  • It is also recommended to test the build process with the new environment variable to ensure that everything is working as expected.

@ti-chi-bot ti-chi-bot bot added the size/XS label Jan 16, 2024
Copy link

ti-chi-bot bot commented Jan 16, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request title suggests that a new environment variable has been added. The description indicates that the purpose of the change is to fix an error related to empty branch/tag info for tidb while building grafana. The diff shows that go run command has been replaced with make command and new environment variables have been added.

Potential problems that can be identified are:

  • It is not clear whether the change has been tested or not.
  • It is not clear why the go run command was replaced with make command.
  • If the TARGET environment variable is not set correctly, it might cause issues with the make command.

Fixing suggestions:

  • The pull request should include information regarding testing that has been performed to ensure that the changes work as intended.
  • The reason for replacing the go run command with the make command should be included in the pull request description.
  • It would be helpful to add some documentation on the purpose of the new environment variables and how to set them correctly.

Copy link

ti-chi-bot bot commented Jan 16, 2024

[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

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

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

ti-chi-bot bot commented Jan 16, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-16 09:39:18.493704805 +0000 UTC m=+262400.058002498: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Jan 16, 2024
@jayl1e jayl1e merged commit 567f4d6 into main Jan 16, 2024
2 checks passed
@jayl1e jayl1e deleted the fix/grafana branch January 16, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants