Skip to content

chore(ci): run linter on proto and grpc modules when librarian.yaml is updated - #14004

Open
zhumin8 wants to merge 1 commit into
googleapis:mainfrom
zhumin8:ci-lint-grpc-proto-on-librarian-change
Open

chore(ci): run linter on proto and grpc modules when librarian.yaml is updated#14004
zhumin8 wants to merge 1 commit into
googleapis:mainfrom
zhumin8:ci-lint-grpc-proto-on-librarian-change

Conversation

@zhumin8

@zhumin8 zhumin8 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This change aim to catch future lint issues when onboarding libraries at the initial PR, while not offsetting regular PR's lint run time.

For googleapis/librarian#7146

@zhumin8
zhumin8 requested review from a team as code owners August 6, 2026 06:51

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request modifies .kokoro/build.sh to conditionally include proto-* and grpc-* modules in the lint check when librarian.yaml is modified. The review feedback suggests improving the robustness of the file detection by using a regular expression to match exact file names and simplifying the complex conditional logic in the directory filtering using idiomatic Bash syntax.

Comment thread .kokoro/build.sh
echo "${changed_file_list}"

has_librarian_change="false"
if grep -q "librarian.yaml" <<< "${changed_file_list}"; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using grep -q "librarian.yaml" can lead to false positives if there are other files containing librarian.yaml as a substring (e.g., not_librarian.yaml or some_librarian.yaml). To ensure we only match files named exactly librarian.yaml (either at the root or in a subdirectory), use a regular expression that matches the start of a line or a slash, followed by librarian.yaml at the end of the line.

Suggested change
if grep -q "librarian.yaml" <<< "${changed_file_list}"; then
if grep -qE "(^|/)librarian\.yaml$" <<< "${changed_file_list}"; then

Comment thread .kokoro/build.sh
Comment on lines +268 to +269
{ [ "${has_librarian_change}" == "true" ] || [[ "$(basename "${dir}")" != "proto-google-"* ]]; } && \
{ [ "${has_librarian_change}" == "true" ] || [[ "$(basename "${dir}")" != "grpc-google-"* ]]; } && \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using { [ ... ] || [[ ... ]]; } is complex and mixes different test syntaxes ([ and [[) along with command grouping { ... }. Since Bash's [[ ... ]] natively supports the || operator, you can simplify these conditions to be much more readable and idiomatic.

Suggested change
{ [ "${has_librarian_change}" == "true" ] || [[ "$(basename "${dir}")" != "proto-google-"* ]]; } && \
{ [ "${has_librarian_change}" == "true" ] || [[ "$(basename "${dir}")" != "grpc-google-"* ]]; } && \
[[ "${has_librarian_change}" == "true" || "$(basename "${dir}")" != "proto-google-"* ]] && \
[[ "${has_librarian_change}" == "true" || "$(basename "${dir}")" != "grpc-google-"* ]] && \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant