Skip to content

Conversation

wuhuizuo
Copy link
Collaborator

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind October 10, 2024 02:23
Copy link

ti-chi-bot bot commented Oct 10, 2024

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

The PR is titled "refactor(prow-jobs): refactor ci prow job kustomization generation" and is signed by "wuhuizuo".

The key changes in this PR are in two files:

  1. .ci/update-prow-job-kustomization.sh: The script used to generate kustomization for prow jobs has been simplified. The key_index variable and its incrementation have been removed, and the key is now generated by replacing "/" with "_" in the filename of the prow job.

  2. prow-jobs/kustomization.yaml: The configMapGenerator in kustomization.yaml has been refactored. The filenames in the files list have been changed from a 3-digit number format (e.g., "001.yaml") to a format that represents the actual file path (e.g., "pingcap-inc_enterprise-extensions_postsubmits.yaml").

Potential Problems:

  1. The key generated by the new method in .ci/update-prow-job-kustomization.sh may be too long or contain special characters which may not be compatible with certain systems.

  2. The change in the key format in prow-jobs/kustomization.yaml may affect the systems or scripts that rely on the old key format.

Fixing Suggestions:

  1. Add a length check for the key and a method to sanitize the key, removing or replacing any special characters.

  2. Check all the systems and scripts that use the keys in prow-jobs/kustomization.yaml. If they rely on the old key format, they should be updated to work with the new format.

@ti-chi-bot ti-chi-bot bot added the size/L label Oct 10, 2024
@wuhuizuo wuhuizuo force-pushed the fix/refactor-ci-prow-job-kustomization branch from 4ce8386 to da3aa1e Compare October 10, 2024 02:25
Copy link

ti-chi-bot bot commented Oct 10, 2024

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

The pull request is titled "refactor(prow-jobs): refactor ci prow job kustomization generation" and is signed by "wuhuizuo".

Key changes in this pull request include:

  1. The script file update-prow-job-kustomization.sh has been updated. The key_index counter variable and related code have been removed. The new method still loops over the *.yaml files in the prow-jobs directory but now forms the key value by replacing the slashes in the filename with underscores using sed.

  2. The kustomization.yaml file under prow-jobs has been updated. The file names under configMapGenerator are no longer numbered (from 001.yaml to 097.yaml). Instead, they are now named by replacing the slashes in the original paths with underscores.

Potential Problems:

  1. The key modification in the configMapGenerator might cause issues if the CI/CD system or any related scripts depend on the old numbered key format.

  2. The change in the key naming convention could lead to confusion or misinterpretation. For example, someone might interpret the underscore as part of the original path, which could lead to difficulties when trying to locate the original file.

Fixing Suggestions:

  1. Ensure that all systems and scripts that interact with this configuration have been updated to work with the new key format, or consider implementing a backward-compatible solution.

  2. To avoid confusion, consider using a different character to replace the slashes, or add a clarifying comment to the kustomization.yaml and/or update-prow-job-kustomization.sh files.

It's also worth noting that the pull request description only contains a signature and doesn't provide any context or reasoning for the changes. It would be good practice for the author to include a brief explanation of why the changes have been made and what impact they're expected to have.

@wuhuizuo wuhuizuo added the lgtm label Oct 10, 2024
@wuhuizuo
Copy link
Collaborator Author

/approve

@ti-chi-bot ti-chi-bot bot added the approved label Oct 10, 2024
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

Copy link

ti-chi-bot bot commented Oct 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 merged commit a4ae42f into main Oct 10, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/refactor-ci-prow-job-kustomization branch October 10, 2024 02:35
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