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: fixate patch version for binlog #402

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Sep 5, 2024

What's changed:

  • fixate the patch version of the artifacts from tidb-binlog repo that will be used in composing offline deployment packages.
  • fixate the patch version of the artifacts from tidb-binlog repo that will be used in build ctl tiup package.
  • disable the image sync rules of tidb-binlog.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
fixate the path version of the artifacts from tidb-binlog repo.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
It's deprecated in v8.3 and removed in v8.4, also none future patch version will be released.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind September 5, 2024 10:35
@ti-chi-bot ti-chi-bot bot added the size/L label Sep 5, 2024
Copy link

ti-chi-bot bot commented Sep 5, 2024

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

The pull request titled "chore: fixate patch version for binlog" contains changes in three files namely delivery.yaml, offline-packages.yaml.tmpl, and packages.yaml.tmpl. Here are the key changes and potential issues:

  1. packages/delivery.yaml: The previous versioning rules for the tidb-binlog image have been commented out, effectively deprecating these rules. It doesn't seem to have been replaced by any new rules. If this was intentional, there's no issue here, but if the tidb-binlog image is still in use, you could be breaking something.

  2. packages/offline-packages.yaml.tmpl and packages/packages.yaml.tmpl: The version of tidb-binlog has been fixed based on a predefined map ($binlog_final_version_map). This means it will no longer use the release version dynamically but will follow the versions defined in the map. This could potentially cause issues if the map isn't updated as new versions are released.

Suggestions:

  1. If the tidb-binlog image is still in use, provide new versioning rules to replace the deprecated ones.
  2. Regularly update the $binlog_final_version_map to ensure it includes all necessary versions. Consider automating this if possible.
  3. Make sure all the changes are well-documented and communicated to the team to avoid confusion.

Copy link

ti-chi-bot bot commented Sep 5, 2024

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

The pull request titled "chore: fixate patch version for binlog" mainly includes two types of changes:

  1. Disabling the image sync rules for the tidb-binlog image in delivery.yaml. This is done by commenting out the related configuration under hub.pingcap.net/pingcap/tidb-binlog/image.

  2. Hard coding version number for tidb-binlog in offline-packages.yaml.tmpl and packages.yaml.tmpl. This is done by creating a dictionary of version mappings ($binlog_final_version_map) and using it to set the $use_binlog_version variable. The version of tidb-binlog is then replaced with $use_binlog_version in various parts of the yaml files.

Potential Issues:

  1. If the binlog version needs to be updated or rolled back in the future, the hardcoded versions in the files will need to be manually updated. This could potentially lead to human error or missed updates.

  2. Disabling the image sync rules for tidb-binlog might impact environments that depend on the image sync. If any services are expecting the tidb-binlog image to be updated regularly, they might not work correctly after this change.

Suggested Fixes:

  1. Instead of hard coding the binlog version, consider implementing a more dynamic way to determine the binlog version, such as using environment variables or config files.

  2. If image sync rules are disabled for tidb-binlog, ensure that all dependent services or environments are updated and tested accordingly to handle this change.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 5, 2024

/cc @Benjamin2037

…tl package

It's deprecated in v8.3 and removed in v8.4, also none future patch version will be released.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the chore/pin-patch-version-for-binlog branch from 932510a to 04ea550 Compare September 5, 2024 10:51
Copy link

ti-chi-bot bot commented Sep 5, 2024

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

Summary of Changes:

  1. The synchronization rules of the tidb-binlog image in the image_copy_rules section of delivery.yaml have been commented out, effectively disabling them.

  2. A version dictionary for tidb-binlog is added to offline-packages.yaml.tmpl and packages.yaml.tmpl files. It maps the release version to a specific binlog version. The binlog version is then used for creating the URLs for various components in the editions section of the offline-packages.yaml.tmpl file and in the components section of the packages.yaml.tmpl file.

Potential Problems:

  1. The tidb-binlog image synchronization is disabled. This may cause problems if other services rely on the sync of tidb-binlog.

  2. The use of a hard-coded dictionary for mapping the release version to a specific binlog version in the offline-packages.yaml.tmpl and packages.yaml.tmpl files is not a scalable solution. If new versions are released, the dictionary needs to be manually updated.

Fixing Suggestions:

  1. If other services rely on the sync of the tidb-binlog image, you need to ensure that those services are updated accordingly or the sync rules are re-enabled.

  2. Consider using a more dynamic method for mapping the release version to a specific binlog version, such as a configuration file or a service that provides the mapping. This will prevent the need for manual updates to the dictionary in the offline-packages.yaml.tmpl and packages.yaml.tmpl files.

  3. Ensure that the version changes are well communicated and documented to avoid confusion for the users.

  4. It's important to add tests to ensure that the URLs are correctly formatted with the new binlog versions.

  5. The pull request should also include updates to any relevant documentation.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 5, 2024

/hold

wait for LGTM from @Benjamin2037

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 Sep 5, 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 Sep 5, 2024

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

The PR titled "chore: fixate patch version for binlog" consists of significant changes in the delivery.yaml, offline-packages.yaml.tmpl, and packages.yaml.tmpl files.

Key Changes:

  1. In delivery.yaml file, the image sync rules of tidb-binlog have been disabled and the related code has been commented out and marked as deprecated.
  2. In offline-packages.yaml.tmpl and packages.yaml.tmpl files, the patch version of the artifacts from the tidb-binlog repo is fixated. This is done by introducing a new map $binlog_final_version_map where specific versions are mapped to their corresponding final versions. This map is used to determine the version of binlog package to be used in various components.

Potential Problems:

  1. There are no apparent logical issues in the changes. However, it's important to consider the implications of fixating the patch version. This could possibly prevent the use of future bug fixes or improvements in tidb-binlog.
  2. Disabling the image sync rules for tidb-binlog might affect the processes or systems that are dependent on these rules. It's not clear from the PR why this change is necessary.

Fixing Suggestions:

  1. Ensure that fixating the patch version of tidb-binlog is necessary and more beneficial than harmful. If newer versions of tidb-binlog are critical for some systems or processes, consider using a more flexible versioning strategy.
  2. Provide a clear explanation for why the image sync rules for tidb-binlog are being disabled. If these rules are required elsewhere, consider implementing an alternative solution.

Lastly, it's crucial to thoroughly test these changes in a controlled environment before merging this PR to avoid any potential issues in production.

@wuhuizuo wuhuizuo force-pushed the chore/pin-patch-version-for-binlog branch from f2503fd to 04cd508 Compare September 5, 2024 12:12
Copy link

ti-chi-bot bot commented Sep 5, 2024

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

Summary of changes:

  1. It fixates the patch version of tidb-binlog artifacts. The version is determined by comparing the release version with a pre-defined version map.
  2. It disables the image sync rules for tidb-binlog by commenting out the entire section related to it.

Potential problems:

  1. If the pre-defined version map is not updated in time, it might lead to usage of outdated version of tidb-binlog.
  2. Commenting out the sync rules for tidb-binlog will stop the synchronization of its images to other registries. If there are services depending on tidb-binlog images from these registries, they might fail.

Fixing suggestions:

  1. Regularly update the version map for tidb-binlog to ensure the used version is up-to-date.
  2. If there are services depending on tidb-binlog images from the registries listed in the sync rules, consider to keep those sync rules or provide an alternative way for those services to retrieve the tidb-binlog images.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 5, 2024

/hold cancel

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the chore/pin-patch-version-for-binlog branch from 04cd508 to de9c2aa Compare September 5, 2024 12:19
Copy link

ti-chi-bot bot commented Sep 5, 2024

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

Summary of Key Changes:

  1. The pull request introduces a change in the delivery.yaml file to disable the image sync rules of tidb-binlog. This is done by commenting out the sync rules corresponding to tidb-binlog.

  2. Another change is introduced in the offline-packages.yaml.tmpl and packages.yaml.tmpl files to fixate the patch version of the artifacts from tidb-binlog repo that will be used in composing offline deployment packages and in building the ctl tiup package. This is achieved by creating a dictionary of the final version map for binlog, checking the release version against the constraints in the dictionary, and assigning the corresponding version to use for binlog. The "{{ .Release.version }}" in the url for tidb-binlog package is replaced with "{{ $use_binlog_version }}".

Potential Problems:

  1. Commenting out the image sync rules for tidb-binlog in the delivery.yaml file might lead to outdated images being used if not handled correctly elsewhere.

  2. The introduced changes in the offline-packages.yaml.tmpl and packages.yaml.tmpl files to fixate the binlog version might cause problems if the provided versions in the dictionary do not exist or are not compatible with the current setup.

Suggested Fixes:

  1. For the first issue, ensure that the new way of handling image updates for tidb-binlog is implemented correctly and tested.

  2. For the second issue, make sure that the versions provided in the dictionary are valid and compatible with the current setup. It would also be helpful to handle any exceptions that might occur if a version is not found or not compatible.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 5, 2024

/approve

Copy link

ti-chi-bot bot commented Sep 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, 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 Sep 5, 2024
@ti-chi-bot ti-chi-bot bot merged commit a73490d into main Sep 5, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the chore/pin-patch-version-for-binlog branch September 5, 2024 12:29
wuhuizuo added a commit that referenced this pull request Sep 6, 2024
This reverts commit a73490d.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
ti-chi-bot bot pushed a commit that referenced this pull request Sep 6, 2024
This reverts commit a73490d.

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

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
wuhuizuo added a commit that referenced this pull request Sep 6, 2024
This reverts commit a73490d.

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

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
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