Added add-key-item / delete-key-item to utils/skip_cla_entry.sh#4942
Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
WalkthroughAdds AWS JSON helper functions and a Python-backed array modifier to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "skip_cla_entry.sh (MODE)"
participant Py as "modify_skip_cla_array_value (Python)"
participant AWS as "AWS CLI"
participant DDB as "DynamoDB"
CLI->>AWS: aws_key_json / get_skip_cla_key_value (query current item)
AWS-->>CLI: current skip_cla value (JSON or missing)
CLI->>Py: invoke modifier (current, action:add|delete, item)
Py-->>CLI: structured JSON {changed,delete_key,new_value}
alt changed == true
CLI->>AWS: aws update-item (set new_value) OR delete-item/attribute
AWS-->>DDB: apply update/delete
AWS-->>CLI: success/failure
else changed == false
CLI-->>CLI: no-op (idempotent)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
lukaszgryglicki
requested review from
ahmedomosanya,
jarias-lfx and
mlehotskylf
There was a problem hiding this comment.
Pull request overview
Adds new add-key-item / delete-key-item modes to utils/skip_cla_entry.sh to idempotently add/remove a single pattern entry within a repository’s skip_cla configuration stored in DynamoDB, addressing the reported Slack issue.
Changes:
- Added two new modes (
add-key-item,delete-key-item) and updated usage/help examples. - Introduced helper functions to safely build DynamoDB JSON arguments via
jqand to modifyskip_clapattern arrays via an embedded Python snippet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
utils/skip_cla_entry.sh (1)
52-60: Consider adding error handling for AWS CLI failures.If the AWS CLI command fails (network error, auth issue, missing permissions), the function silently returns empty, which is then treated the same as "key not found". This could mask errors and lead to unexpected behavior (e.g.,
add-key-itemwould create a new single-item array instead of adding to existing).💡 Optional: Check AWS exit code
get_skip_cla_key_value() { local org="$1" local repo="$2" - aws --profile "lfproduct-${STAGE}" --region "${REGION}" dynamodb get-item \ + local output + output=$(aws --profile "lfproduct-${STAGE}" --region "${REGION}" dynamodb get-item \ --table-name "cla-${STAGE}-github-orgs" \ --key "$(aws_key_json "$org")" \ - --projection-expression 'skip_cla' \ - | jq -r --arg repo "$repo" '.Item.skip_cla.M[$repo].S // empty' + --projection-expression 'skip_cla' 2>&1) || { + echo "Error fetching from DynamoDB: $output" >&2 + return 1 + } + echo "$output" | jq -r --arg repo "$repo" '.Item.skip_cla.M[$repo].S // empty' }Then check the return value at line 163.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/skip_cla_entry.sh` around lines 52 - 60, The get_skip_cla_key_value function currently treats any AWS CLI failure as "key not found"; modify it to detect and handle AWS CLI errors by checking the aws command exit status (or capturing stderr) after the dynamodb get-item call in get_skip_cla_key_value, and propagate or surface failures instead of returning empty: on non-zero exit, write an informative error message (including the AWS CLI stderr and context like org/repo and table name) to stderr and return a non-success exit code (or a distinct error value) so callers (e.g., add-key-item) can distinguish real errors from a missing key; ensure you reference get_skip_cla_key_value when adding this check and adjust callers to handle the error signal accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/skip_cla_entry.sh`:
- Around line 52-60: The get_skip_cla_key_value function currently treats any
AWS CLI failure as "key not found"; modify it to detect and handle AWS CLI
errors by checking the aws command exit status (or capturing stderr) after the
dynamodb get-item call in get_skip_cla_key_value, and propagate or surface
failures instead of returning empty: on non-zero exit, write an informative
error message (including the AWS CLI stderr and context like org/repo and table
name) to stderr and return a non-success exit code (or a distinct error value)
so callers (e.g., add-key-item) can distinguish real errors from a missing key;
ensure you reference get_skip_cla_key_value when adding this check and adjust
callers to handle the error signal accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30f90dd2-efd8-4ca3-b9fd-59146846a151
📒 Files selected for processing (1)
utils/skip_cla_entry.sh
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
utils/skip_cla_entry.sh (2)
73-114: Verify that format normalization to array syntax is intentional.The Python helper normalizes single values to array format. For example, if the current value is
pattern(not wrapped), after any modification it becomes[pattern]. This ensures consistent formatting going forward, but changes the stored representation.If this is intentional for consistency, consider documenting this normalization behavior. Otherwise, preserve the original format when there's only one item:
♻️ Optional: preserve original format for single items
if not items: new_value = "" +elif len(items) == 1: + new_value = items[0] else: new_value = "[" + "||".join(items) + "]"
166-232: Read-modify-write has potential race condition.The code fetches the current value (line 175), modifies it in Python, then writes it back (lines 214-219 or 225-230). If two operators run this script concurrently on the same org/repo, one update could be lost.
For a utility script run manually, this is typically acceptable. If concurrent usage becomes a concern, DynamoDB conditional expressions can provide optimistic locking.
ℹ️ Optional: Add conditional update to prevent lost updates
You could use
--condition-expressionto ensure the value hasn't changed:--condition-expression 'skip_cla.#repo = :current_val' --expression-attribute-values '{ ":val": {"S": "new"}, ":current_val": {"S": "old"} }'This would fail with
ConditionalCheckFailedExceptionif another process modified the value, allowing retry logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/skip_cla_entry.sh` around lines 166 - 232, The read-modify-write in the add-key-item/delete-key-item branch (using get_skip_cla_key_value -> modify_skip_cla_array_value -> aws update-item with new_value/delete_key) can lose updates if concurrent runs occur; modify the aws update-item calls to include a --condition-expression that checks the current stored value equals the fetched current_value (use expression-attribute-values to pass :current_val and :val) so the update fails if the item changed, and handle the conditional-failure by retrying or surfacing a clear error (e.g., detect ConditionalCheckFailedException from aws CLI and either retry the read-modify-write or exit with a helpful message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/skip_cla_entry.sh`:
- Around line 166-232: The read-modify-write in the add-key-item/delete-key-item
branch (using get_skip_cla_key_value -> modify_skip_cla_array_value -> aws
update-item with new_value/delete_key) can lose updates if concurrent runs
occur; modify the aws update-item calls to include a --condition-expression that
checks the current stored value equals the fetched current_value (use
expression-attribute-values to pass :current_val and :val) so the update fails
if the item changed, and handle the conditional-failure by retrying or surfacing
a clear error (e.g., detect ConditionalCheckFailedException from aws CLI and
either retry the read-modify-write or exit with a helpful message).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eafbf9d-854c-4f5a-bb49-0ee856aae68d
📒 Files selected for processing (1)
utils/skip_cla_entry.sh
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
utils/skip_cla_entry.sh (2)
200-203: Consider using-ninstead of! -zfor consistency.
[ -n "$DEBUG" ]is more idiomatic than[ ! -z "$DEBUG" ]. This applies to lines 200, 211, 218, and 229.Example diff
- if [ ! -z "$DEBUG" ]; then + if [ -n "$DEBUG" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/skip_cla_entry.sh` around lines 200 - 203, Replace the non-idiomatic conditional tests that use [ ! -z "$DEBUG" ] with the more idiomatic [ -n "$DEBUG" ] everywhere in utils/skip_cla_entry.sh (e.g., the DEBUG checks shown in the snippet and the other DEBUG branches referenced in the review), updating each if conditional (and any corresponding elif/while tests if present) so they consistently use -n to test non-empty DEBUG.
101-105: Consider renaming the loop variable to avoid shadowing.The loop variable
itemshadows the conceptual "item" input (stored initem_raw). While not a bug, using a distinct name likeentryorvalwould improve readability.Suggested rename
if action == "add": - for item in item_values: - if item not in items: - items.append(item) + for entry in item_values: + if entry not in items: + items.append(entry) changed = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/skip_cla_entry.sh` around lines 101 - 105, The loop variable `item` in the add branch shadows the input concept `item_raw`; rename the loop variable (e.g., to `entry` or `val`) in the block that iterates `item_values` so it no longer collides with `item_raw`, updating references inside the loop where `items.append(item)` and the membership check `if item not in items` to use the new name, keeping the surrounding variables `action`, `item_values`, `items`, and `changed` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/skip_cla_entry.sh`:
- Around line 200-203: Replace the non-idiomatic conditional tests that use [ !
-z "$DEBUG" ] with the more idiomatic [ -n "$DEBUG" ] everywhere in
utils/skip_cla_entry.sh (e.g., the DEBUG checks shown in the snippet and the
other DEBUG branches referenced in the review), updating each if conditional
(and any corresponding elif/while tests if present) so they consistently use -n
to test non-empty DEBUG.
- Around line 101-105: The loop variable `item` in the add branch shadows the
input concept `item_raw`; rename the loop variable (e.g., to `entry` or `val`)
in the block that iterates `item_values` so it no longer collides with
`item_raw`, updating references inside the loop where `items.append(item)` and
the membership check `if item not in items` to use the new name, keeping the
surrounding variables `action`, `item_values`, `items`, and `changed` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6881277-64fd-49c0-a4f2-721ba6067935
📒 Files selected for processing (1)
utils/skip_cla_entry.sh
lukaszgryglicki
deleted the
unicron-add-support-for-adding-deleting-array-items-of-skip-cla-entries
branch
Fixes slack report.
Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot