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

chore(packages): remove arbiter from offline deployment packages #386

Merged
merged 10 commits into from
Aug 22, 2024

Conversation

wuhuizuo
Copy link
Contributor

Starts from v8.1.1

Signed-off-by: wuhuizuo wuhuizuo@126.com

Starts from v8.1.1

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind August 20, 2024 06:22
Copy link

ti-chi-bot bot commented Aug 20, 2024

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

Based on the pull request, the key changes are:

  • Remove the arbiter package from the offline deployment packages.
  • Add a check constraint to exclude the arbiter package for versions less than 8.1.1-0.

There are no potential problems identified in this pull request. However, it's always a good practice to test the changes before merging them into the main branch.

As for fixing suggestions, it would be helpful to add a brief explanation in the pull request description about why arbiter is being removed from offline deployment packages. Additionally, it would be good to provide instructions on how to test the changes.

@ti-chi-bot ti-chi-bot bot added the size/XS label Aug 20, 2024
@wuhuizuo
Copy link
Contributor Author

/hold
/cc @Benjamin2037

@wuhuizuo
Copy link
Contributor Author

it should merge after release-8.3 is released.

Copy link

@Benjamin2037 Benjamin2037 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

ti-chi-bot bot commented Aug 22, 2024

@Benjamin2037: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Summary:
This pull request removes arbiter from the offline deployment packages. It adds an if condition to exclude the arbiter package from the offline deployment packages if the version is less than 8.1.1-0.

Potential problems:
It seems that the author has carefully handled the changes in this pull request and there are no potential problems.

Fixing suggestions:
No fixing suggestions are needed. The pull request is ready to merge.

packages/packages.yaml.tmpl Outdated Show resolved Hide resolved
Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Based on the pull request title and diff, the key change is the removal of arbiter from offline deployment packages. The change is made by adding an if statement that checks the version number, and only includes arbiter in packages for versions lower than 8.1.1-0.

There doesn't seem to be any obvious potential problems with this pull request. However, one suggestion for improvement would be to add a clear explanation in the description of why arbiter is being removed and why the version number is being used in the if statement. This can help other contributors understand the reasoning behind the change.

Another suggestion would be to add some testing to ensure that the offline deployment packages are built correctly after the removal of arbiter. This can help catch any potential issues that may arise from the change.

Overall, this is a straightforward pull request with no obvious issues.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Summary:
The pull request removes the arbiter package from offline deployment packages if the TiDB version is less than 8.1.1-0. This is done by adding an if condition to the package definition.

Potential problems:
It's hard to identify potential problems without knowing the context of the project and the purpose of the arbiter package. However, if the arbiter package is necessary for some TiDB versions, removing it can cause issues for users who are using those versions.

Fixing suggestions:
If the arbiter package is not necessary for any TiDB version, then the pull request is fine. Otherwise, it's better to keep the package and provide a reason why it's being removed for certain versions. For example, if it's a deprecated package, then a comment can be added to explain why it's being removed.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

The key changes in this pull request are that it removes the arbiter package from the offline deployment packages and adds a conditional statement to only include the arbiter package in offline packages if the version is less than 8.1.1-0.

One potential problem with this change is that if arbiter is necessary for offline deployment with a version greater than or equal to 8.1.1-0, it will not be included in the offline packages.

To fix this issue, the conditional statement should be updated to include the arbiter package if the version is greater than or equal to 8.1.1-0.

Overall, this pull request seems like a minor change and is unlikely to cause any major issues.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Based on the pull request title and description, it seems that the arbiter package is being removed from offline deployment packages. Looking at the diff, it appears that the removal is conditional based on the version number of the release.

The key changes made in this pull request are:

  • The removal of the arbiter package from the offline deployment packages.
  • The addition of conditions based on the version number of the release.

There don't seem to be any potential problems with this pull request as long as the conditions for the package removal are accurate.

As for fixing suggestions, I would suggest adding more details to the pull request description to provide more context for the changes being made. It would also be helpful to have more information on why this change is being made and what impact it will have on the overall system.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Based on the pull request title and description, the changes seem to be related to removing the "arbiter" component from offline deployment packages.

Looking at the diff, it appears that the "if" statement has been added to several instances of the "arbiter" component, indicating that it should only be included in certain versions of the release. Specifically, it should be included in versions prior to 8.1.1-0 or versions 8.2.0-0 and later.

Overall, the changes seem reasonable and do not appear to introduce any major problems. However, it might be useful to add a comment explaining the reasoning behind the version constraints, in case future contributors are confused by the change.

One minor suggestion would be to ensure that the code formatting is consistent throughout the diff. There are a few lines where the spacing is different than the rest of the file, which could be cleaned up for readability.

Other than that, I think the pull request looks good and can be merged.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Based on the pull request title and description, it seems that the arbiter package is being removed from offline deployment packages. Looking at the diff, we can see that the if statement is being added for the arbiter package in multiple places. This if statement checks if the current release version is less than 8.1.1-0 or greater than or equal to 8.2.0-0. If this condition is true, then the arbiter package will not be included in the offline deployment packages.

It is important to note that the if statement is written in the Go language and uses the semver package for version comparison.

As for potential problems, it is possible that the if statement logic is incorrect and could result in unexpected behavior. It is also possible that removing the arbiter package could cause issues for users who rely on it.

As for fixing suggestions, it would be good to thoroughly test the changes to ensure that the logic is correct and that there are no unexpected issues. It would also be good to communicate the removal of the arbiter package to users and provide alternative solutions if necessary.

Copy link

ti-chi-bot bot commented Aug 22, 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 description, it seems that this PR intends to remove the arbiter package from offline deployment packages. Looking at the diff, it seems that the change is adding an if condition to only include the arbiter package in the deployment packages if the version is less than 8.1.1-0 or greater than or equal to 8.2.0-0.

There don't seem to be any potential problems with the change as it is a straightforward removal of a package based on version constraints.

As for fixing suggestions, it seems that the change has been implemented correctly and there are no issues with the code. However, it would be good to check if there are any dependencies or downstream systems that may be affected by this change. Additionally, it may be worth updating the PR description to provide more context on why the arbiter package is being removed and what the impact of this change is.

Copy link

ti-chi-bot bot commented Aug 22, 2024

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

Based on the pull request title and description, it seems that the author is removing the arbiter package from the offline deployment packages. The changes can be seen in the packages/offline-packages.yaml.tmpl and packages/packages.yaml.tmpl files.

In the offline-packages.yaml.tmpl file, the if statement is added for the arbiter package, which checks if the version is less than 8.1.1-0 or greater than or equal to 8.2.0-0. This means that the arbiter package will not be included in the offline package if the version is within this range. In the packages.yaml.tmpl file, the same if statement is added to exclude the arbiter package.

The changes seem reasonable and straightforward. However, it would be better if the author provided some context or explanation in the pull request description. It is also good to check if the if statement works as expected.

One suggestion to improve the pull request is to provide more context in the description. For example, why is the arbiter package being removed, and what are the implications of this change?

Another suggestion is to test the changes thoroughly to ensure that the if statement works as expected. The author can test different versions of the software to verify that the arbiter package is excluded or included correctly.

Overall, the pull request seems good, and the changes are straightforward. The author can provide more context and test the changes to ensure that everything works as expected.

@wuhuizuo
Copy link
Contributor Author

/unhold

Copy link
Contributor

@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 Aug 22, 2024
Copy link

ti-chi-bot bot commented Aug 22, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-08-22 08:42:54.220427203 +0000 UTC m=+427769.354877324: ☑️ agreed by purelind.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Aug 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, purelind, 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 approved label Aug 22, 2024
@ti-chi-bot ti-chi-bot bot merged commit fce16d5 into main Aug 22, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the chore/update-offline-config branch August 22, 2024 08:46
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.

3 participants