Skip to content

Comments

refactor(scripts): improve robustness and readability of lsync.sh#51413

Merged
lmktfy merged 1 commit intokubernetes:mainfrom
samzong:refactor/improve-lsync-script
Jun 30, 2025
Merged

refactor(scripts): improve robustness and readability of lsync.sh#51413
lmktfy merged 1 commit intokubernetes:mainfrom
samzong:refactor/improve-lsync-script

Conversation

@samzong
Copy link
Member

@samzong samzong commented Jun 28, 2025

At this PR, I refactor scripts/lsync.sh to make more robust and align shell scripting best practices.

  • Handles special filenames: The script now correctly processes filenames containing spaces or other special characters by using a find ... -print0 | while read loop.
  • Modernized syntax: Replaced legacy backticks (`...`) with $(...)
  • Simplified conditionals: Use if were simplified to check command exit codes, cleaner and more readable.

Special: Improvements the script without changing its core functionality.

use https://www.shellcheck.net for find this issues.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jun 28, 2025
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 28, 2025
Signed-off-by: samzong <samzong.lu@gmail.com>
@samzong samzong force-pushed the refactor/improve-lsync-script branch from 88cc2ee to 50e256e Compare June 28, 2025 18:36
Copy link
Member

@lmktfy lmktfy left a comment

Choose a reason for hiding this comment

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

/lgtm

Nits are tiny.

SYNCED=0
fi
done
done < <(find "$1" -name "*.md" -print0)
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would add -type f here.

# Try get the English version
EN_VERSION=`echo $LOCALIZED | sed "s/content\/.\{2,5\}\//content\/en\//g"`
if [ ! -e $EN_VERSION ]; then
EN_VERSION=$(echo "$LOCALIZED" | sed "s/content\/.\{2,5\}\//content\/en\//g")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EN_VERSION=$(echo "$LOCALIZED" | sed "s/content\/.\{2,5\}\//content\/en\//g")
EN_VERSION="$(echo "$LOCALIZED" | sed "s/content\/.\{2,5\}\//content\/en\//g")"

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

DetailsGit tree hash: f9a91f56c7c53d61a12ef3b3c1f4219e6ab418f4

@lmktfy
Copy link
Member

lmktfy commented Jun 30, 2025

Let's merge. If there are snags we can revert; we can also iterate on this for further improvements.

@lmktfy
Copy link
Member

lmktfy commented Jun 30, 2025

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lmktfy

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

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2025
@lmktfy lmktfy merged commit 8f776b4 into kubernetes:main Jun 30, 2025
1 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants