Add image-org-cleanup to periodic pipeline (also move rosa & jenkins cleanup)#1774
Open
chetna14manku wants to merge 1 commit into
Open
Add image-org-cleanup to periodic pipeline (also move rosa & jenkins cleanup)#1774chetna14manku wants to merge 1 commit into
chetna14manku wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chetna14manku The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds four new cleanup scripts (Artifactory, Nexus, Quay, Jenkins), updates periodic cleanup pipeline to run them and a ROSA cluster cleanup task, extends the download-and-execute task to mount credentials and export ROSA/AWS env vars, and adjusts git cleanup scripts to accept uppercase characters and changes GitLab project ordering to sort by name. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as "Cleanup Script\n(runner)"
participant Cred as "Credentials\n(secret volume / file)"
participant API as "Registry / Jenkins\nHTTP API"
Note over Script,Cred: load token/credentials (env or file)
Script->>Cred: read token / config
Script->>API: GET list (AQL / components / repos / jobs)
API-->>Script: JSON results
Script->>Script: filter by name regex and cutoff date
alt items older than cutoff
Script->>API: DELETE item
API-->>Script: 200/204 or error
else dry-run or no deletion
Script-->>Script: log would-be deletion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
integration-tests/tasks/download-and-execute-cleanup.yaml (1)
52-58: Consider validating extracted credentials before use.If the JSON file is malformed or the keys are missing,
jq -rreturns the literal string "null". This could cause confusing failures in the downstreamdelete-rosa-clusters.shscript. Consider adding validation:export ROSA_TOKEN=$(jq -r '.aws["rosa-hcp"]["rosa-token"]' "$CREDENTIALS_FILE") export AWS_ACCESS_KEY_ID=$(jq -r '.aws["access-key-id"]' "$CREDENTIALS_FILE") export AWS_SECRET_ACCESS_KEY=$(jq -r '.aws["access-key-secret"]' "$CREDENTIALS_FILE") export AWS_SUBNET_IDS=$(jq -r '.aws["rosa-hcp"]["subnets-ids"]' "$CREDENTIALS_FILE") export AWS_DEFAULT_REGION=$(jq -r '.aws["region"]' "$CREDENTIALS_FILE") + + # Validate required credentials were extracted + if [[ -z "$ROSA_TOKEN" || "$ROSA_TOKEN" == "null" || -z "$AWS_ACCESS_KEY_ID" || "$AWS_ACCESS_KEY_ID" == "null" ]]; then + echo "Error: Failed to extract required credentials from $CREDENTIALS_FILE" + exit 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tasks/download-and-execute-cleanup.yaml` around lines 52 - 58, Validate the extracted credential variables (ROSA_TOKEN, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SUBNET_IDS, AWS_DEFAULT_REGION) after reading from CREDENTIALS_FILE: check for empty or "null" values and fail fast with a clear error message listing the missing keys and non-zero exit code before calling delete-rosa-clusters.sh so downstream scripts don't run with invalid credentials.hack/ci/image-org-cleanup/nexus-image-cleanup.sh (2)
69-71: Missing pagination support for Nexus components API.The Nexus REST API returns paginated results with a
continuationToken. This script only fetches the first page, so repositories with many components may not be fully cleaned. Consider implementing pagination:# Example pagination pattern continuation_token="" while true; do api_url="$NEXUS_URL/service/rest/v1/components?repository=$NEXUS_REPOSITORY" if [[ -n "$continuation_token" ]]; then api_url="${api_url}&continuationToken=$continuation_token" fi # ... fetch and process ... continuation_token=$(echo "$components" | jq -r '.continuationToken // empty') if [[ -z "$continuation_token" ]]; then break fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh` around lines 69 - 71, The script only sets api_url="$NEXUS_URL/service/rest/v1/components?repository=$NEXUS_REPOSITORY" and fetches a single page, so implement Nexus pagination using the continuationToken: loop fetching pages for the components endpoint (use the api_url and the same NEXUS_URL/NEXUS_REPOSITORY variables), append &continuationToken=$continuation_token when present, parse continuationToken from the JSON response (jq -r '.continuationToken // empty') and break when empty, and ensure you aggregate/process components from each page rather than only the first page.
123-124: Stale comment references removed pattern matching.Similar to other cleanup scripts, the comment mentions "matches pattern" but no pattern matching is implemented.
- # Check if component matches pattern and is old enough + # Check if component is old enough for cleanup if [[ $last_modified_time -lt $cutoff_time ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh` around lines 123 - 124, The comment above the cleanup conditional is stale—remove or update the phrase about "matches pattern" because no pattern matching is performed; locate the comment immediately preceding the if [[ $last_modified_time -lt $cutoff_time ]] check (variables last_modified_time and cutoff_time) and change it to accurately state that the block only checks age against cutoff_time (or implement the intended pattern matching logic by adding the pattern test before this if). Ensure the comment matches the actual behavior of the conditional.hack/ci/image-org-cleanup/artifactory-image-cleanup.sh (2)
66-68: Comment inconsistent with actual limit.The comment says "limited to 100" but the AQL query uses
.limit(200).# Use AQL (Artifactory Query Language) to find items - # This finds all Docker images in the repository, limited to 100 + # This finds all Docker images in the repository, limited to 200 aql_query='items.find({"repo":"'"$ARTIFACTORY_REPOSITORY"'","type":"folder","depth":1}).include("name","created","modified").limit(200)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 66 - 68, Comment and code disagree: the comment says "limited to 100" but the AQL assigned to the aql_query variable uses .limit(200). Update for consistency by either changing the comment to state "limited to 200" or adjust the AQL (.limit(200)) to .limit(100). Locate the aql_query assignment and modify the comment or the .limit value accordingly so the comment matches the actual .limit used.
68-69: Consider adding pagination or explicit sorting for large repositories.The AQL query has a hard limit of 200 items without pagination. If the repository contains more items, some stale artifacts may not be cleaned up. Consider either:
- Adding explicit sort by
createdormodifiedascending to ensure oldest items are processed first- Implementing pagination with continuation tokens
- aql_query='items.find({"repo":"'"$ARTIFACTORY_REPOSITORY"'","type":"folder","depth":1}).include("name","created","modified").limit(200)' + aql_query='items.find({"repo":"'"$ARTIFACTORY_REPOSITORY"'","type":"folder","depth":1}).include("name","created","modified").sort({"$asc":["modified"]}).limit(200)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 68 - 69, The AQL query stored in aql_query limits results to 200 without ordering or pagination, so artifactory cleanup can miss older items; update the query built in aql_query to include an explicit sort (e.g., .sort({"$asc":["created"]}) or .sort({"$asc":["modified"]})) and increase/remove the limit, and/or implement pagination by looping requests against api_url using AQL with offset/limit (or Artifactory continuation tokens) to fetch all items for the given ARTIFACTORY_REPOSITORY until no more results are returned; ensure the cleanup loop processes items in the intended order (oldest first) and handles subsequent pages.hack/ci/git-org-cleanup/bitbucket-repos-cleanup.sh (1)
112-113: Stale comment references removed functionality.The comment says "Check if repository matches pattern and is old enough" but pattern matching was removed. Update the comment to reflect the current timestamp-only filtering logic.
- # Check if repository matches pattern and is old enough + # Check if repository is old enough for cleanup if [[ $updated_time -lt $cutoff_time ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/git-org-cleanup/bitbucket-repos-cleanup.sh` around lines 112 - 113, Update the outdated comment above the timestamp check: replace the text "Check if repository matches pattern and is old enough" with a concise comment that reflects the current logic — e.g. "Check if repository is older than cutoff_time" — so it references the timestamp-only filtering using the existing variables updated_time and cutoff_time in the conditional (if [[ $updated_time -lt $cutoff_time ]]).hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh (1)
46-52: Consider documenting the global state machine.The global variables (
DELETE_FOLDER,BUILDS,FOLDER,LAST_MOD,INDENT) form a state machine that persists across recursive calls. While the logic appears intentional for cascading folder deletion decisions, adding comments explaining when and why each variable is set/reset would improve maintainability.# Global variables for folder processing +# State machine for recursive job/folder processing: +# - INDENT: Tracks recursion depth for logging (6 spaces per level) +# - DELETE_FOLDER: Whether current item tree should be deleted (reset at top level) +# - BUILDS: Whether any builds were found in current job tree +# - FOLDER: Whether current item is an empty folder (for --empty-folders flag) +# - LAST_MOD: Most recent modification timestamp found in current tree +# - CHECK_DATE: Cutoff timestamp computed from DAYS INDENT=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh` around lines 46 - 52, Add comments documenting the global state machine formed by INDENT, DELETE_FOLDER, BUILDS, FOLDER, and LAST_MOD: annotate the variable block to explain each variable's purpose and valid values, and add inline comments at every place in the code where those variables are modified or reset (e.g., in the recursive folder traversal functions) to explain the conditions that set DELETE_FOLDER to "true"/"false", when BUILDS/FOLDER flags are flipped, how LAST_MOD is updated/compared, and how INDENT is adjusted for recursion; reference these symbols (INDENT, DELETE_FOLDER, BUILDS, FOLDER, LAST_MOD) so future readers understand the lifecycle and reset points during recursive calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 72-77: The Jenkins delete branch in jenkins-cleanup.sh currently
calls curl without checking the HTTP response, so update the deletion logic
around the curl "${URL}doDelete" call to capture the HTTP status (e.g., use curl
-w "%{http_code}" -o /dev/null -s), then check the status code and log an error
if it's not a 2xx response; ensure this uses the existing variables
(JENKINS_USERNAME, JENKINS_API_TOKEN, URL, dry_run, NAME, MOD_DATE) and prints a
clear failure message (similar to the printf lines) and returns non-zero or
records failure instead of silently continuing.
In `@integration-tests/tasks/download-and-execute-cleanup.yaml`:
- Around line 48-58: The if-statement using a subshell is invalid: replace the
line using parentheses with a proper string test, e.g. change the conditional
around SCRIPT_NAME (the check for "delete-rosa-clusters.sh") to use [[ ]] or [ ]
and quote the variable; update the if that guards the CREDENTIALS_FILE export
block so it reads a quoted comparison of "$SCRIPT_NAME" to
"delete-rosa-clusters.sh" and retains the existing then…fi body.
---
Nitpick comments:
In `@hack/ci/git-org-cleanup/bitbucket-repos-cleanup.sh`:
- Around line 112-113: Update the outdated comment above the timestamp check:
replace the text "Check if repository matches pattern and is old enough" with a
concise comment that reflects the current logic — e.g. "Check if repository is
older than cutoff_time" — so it references the timestamp-only filtering using
the existing variables updated_time and cutoff_time in the conditional (if [[
$updated_time -lt $cutoff_time ]]).
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 66-68: Comment and code disagree: the comment says "limited to
100" but the AQL assigned to the aql_query variable uses .limit(200). Update for
consistency by either changing the comment to state "limited to 200" or adjust
the AQL (.limit(200)) to .limit(100). Locate the aql_query assignment and modify
the comment or the .limit value accordingly so the comment matches the actual
.limit used.
- Around line 68-69: The AQL query stored in aql_query limits results to 200
without ordering or pagination, so artifactory cleanup can miss older items;
update the query built in aql_query to include an explicit sort (e.g.,
.sort({"$asc":["created"]}) or .sort({"$asc":["modified"]})) and increase/remove
the limit, and/or implement pagination by looping requests against api_url using
AQL with offset/limit (or Artifactory continuation tokens) to fetch all items
for the given ARTIFACTORY_REPOSITORY until no more results are returned; ensure
the cleanup loop processes items in the intended order (oldest first) and
handles subsequent pages.
In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh`:
- Around line 69-71: The script only sets
api_url="$NEXUS_URL/service/rest/v1/components?repository=$NEXUS_REPOSITORY" and
fetches a single page, so implement Nexus pagination using the
continuationToken: loop fetching pages for the components endpoint (use the
api_url and the same NEXUS_URL/NEXUS_REPOSITORY variables), append
&continuationToken=$continuation_token when present, parse continuationToken
from the JSON response (jq -r '.continuationToken // empty') and break when
empty, and ensure you aggregate/process components from each page rather than
only the first page.
- Around line 123-124: The comment above the cleanup conditional is stale—remove
or update the phrase about "matches pattern" because no pattern matching is
performed; locate the comment immediately preceding the if [[
$last_modified_time -lt $cutoff_time ]] check (variables last_modified_time and
cutoff_time) and change it to accurately state that the block only checks age
against cutoff_time (or implement the intended pattern matching logic by adding
the pattern test before this if). Ensure the comment matches the actual behavior
of the conditional.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 46-52: Add comments documenting the global state machine formed by
INDENT, DELETE_FOLDER, BUILDS, FOLDER, and LAST_MOD: annotate the variable block
to explain each variable's purpose and valid values, and add inline comments at
every place in the code where those variables are modified or reset (e.g., in
the recursive folder traversal functions) to explain the conditions that set
DELETE_FOLDER to "true"/"false", when BUILDS/FOLDER flags are flipped, how
LAST_MOD is updated/compared, and how INDENT is adjusted for recursion;
reference these symbols (INDENT, DELETE_FOLDER, BUILDS, FOLDER, LAST_MOD) so
future readers understand the lifecycle and reset points during recursive calls.
In `@integration-tests/tasks/download-and-execute-cleanup.yaml`:
- Around line 52-58: Validate the extracted credential variables (ROSA_TOKEN,
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SUBNET_IDS, AWS_DEFAULT_REGION)
after reading from CREDENTIALS_FILE: check for empty or "null" values and fail
fast with a clear error message listing the missing keys and non-zero exit code
before calling delete-rosa-clusters.sh so downstream scripts don't run with
invalid credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed13b7d2-764a-4f96-8a56-7b4d7aa0fedc
📒 Files selected for processing (8)
hack/ci/git-org-cleanup/bitbucket-repos-cleanup.shhack/ci/git-org-cleanup/gitlab-repos-cleanup.shhack/ci/image-org-cleanup/artifactory-image-cleanup.shhack/ci/image-org-cleanup/nexus-image-cleanup.shhack/ci/image-org-cleanup/quay-image-cleanup.shhack/ci/service-jenkins-cleanup/jenkins-cleanup.shintegration-tests/pipelines/periodic-cleanup-pipeline.yamlintegration-tests/tasks/download-and-execute-cleanup.yaml
chetna14manku
force-pushed
the
RHTAP-6210
branch
from
d44c6fa to
21e4e52
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh (1)
72-75:⚠️ Potential issue | 🟡 MinorCheck the delete response instead of assuming success.
Line 74 ignores the HTTP status from
doDelete, so 403/404/500 responses are reported as successful deletes. Please capture the status and fail or log the item as not deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh` around lines 72 - 75, The script currently assumes success after the curl POST to "${url}doDelete"; modify the deletion block (when dry_run == "false") to capture curl's HTTP status (e.g., via curl -w "%{http_code}" or --fail with exit code) for the POST using the same credentials (JENKINS_USERNAME/JENKINS_API_TOKEN) and the "${url}doDelete" target, then check the status: log or print a failure message and mark the item as not deleted when the status is not a 2xx (or when curl exits non‑zero), and only print the "Deleting" success line on confirmed success; ensure non‑2xx responses cause the script to either record the failure or exit as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 66-69: The AQL query stored in aql_query currently hardcodes
.limit(200) and the comment incorrectly says "limited to 100", which means the
script only processes the first page of results; update the logic around
aql_query and the loop that posts to api_url to implement pagination: make the
limit configurable (or keep a sensible default) and add an offset parameter
(.offset(N)) that you increment in a loop calling api_url until no items are
returned, processing each page in turn; also correct the comment to reflect the
actual default limit and ensure variables ARTIFACTORY_REPOSITORY and
ARTIFACTORY_URL are used unchanged when building each paginated query.
- Around line 124-128: item_modified_time is set to 0 on parse failure which
makes malformed timestamps appear older than cutoff_time and get deleted; change
the parsing so a failed date conversion does not default to 0 — detect failure
(e.g. capture date exit status or leave item_modified_time empty on error), log
a warning referencing item_modified and item_modified_time, and skip the age
check/delete for that item instead of treating it as epoch; update the
conditional that compares item_modified_time to cutoff_time to only run when
item_modified_time is a valid numeric timestamp.
In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh`:
- Around line 69-76: The current fetch only calls the Nexus components API once
(api_url) and assigns output to tmp_file/components, missing paginated pages;
change to a loop that calls /service/rest/v1/components repeatedly, parsing each
response for .items and .continuationToken, appending &continuationToken=<token>
to api_url for subsequent requests until continuationToken is null, accumulating
all items into the components variable (or a temp file) before the
deletion/processing loop so the code that iterates components later processes
every page; use the existing tmp_file/http_status pattern for each request and
update tmp_file per iteration, and ensure continuationToken parsing uses jq or
similar to extract the token and merge items.
- Around line 120-124: The script currently sets last_modified_time to 0 when
date fails to parse last_modified, which causes malformed timestamps to be
treated as very old and deleted; modify the logic around last_modified_time (and
the last_modified variable) so that if date parsing fails you do not default to
epoch 0 but instead detect the failure, log a warning including the component
identifier and the raw last_modified value, and skip that component (do not
proceed to the deletion block that uses cutoff_time); update the conditional
that compares last_modified_time to cutoff_time to only run deletion when
parsing succeeded.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Around line 127-145: The script currently treats an unparseable last_modified
as "0" (Unix epoch) which makes repositories appear ancient; instead, when
converting last_modified to last_modified_time (variable last_modified_time via
date -d), detect parse failure and skip that repository (log a message and
continue) rather than using 0; update the logic around
last_modified_time/last_modified_date and the cutoff_time check (referencing
last_modified_time, last_modified_date, cutoff_time, repo_name, dry_run,
delete_url) so that if date parsing fails you do not proceed to deletion but
continue to the next repo.
- Around line 115-120: The repo_detail API call is missing the includeTags query
so .tags can be null; update the construction of repo_detail_url (where
QUAY_URL, QUAY_ORGANIZATION and repo_name are combined) to request tags (e.g.,
append ?includeTags=true) so repo_detail contains .tags before computing
last_modified (used by last_modified=$(echo "$repo_detail" | jq -r
'[.tags[].last_modified // empty] | ...')); alternatively, replace the
repo_detail/tag fetch by calling the repository tag endpoint
(/api/v1/repository/{namespace}/{repo}/tag/) and populate repo_detail or
directly derive last_modified from that response.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 198-202: The warning branch for unhandled Jenkins item classes
prints a message while `indent` may be empty and then unconditionally does
`indent=${indent::-6}`, which can cause a "substring expression < 0" fatal
error; fix by only decrementing `indent` when it contains at least six
characters (or is non-empty) after the warning, or use a safe conditional
fallback so the `indent` slice is skipped when empty. Update the else-warning
block that prints "WARNING: Unhandled class ${class}" to guard the subsequent
`indent` modification (variable `indent`) so the script never performs a
negative-length substring operation.
---
Duplicate comments:
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 72-75: The script currently assumes success after the curl POST to
"${url}doDelete"; modify the deletion block (when dry_run == "false") to capture
curl's HTTP status (e.g., via curl -w "%{http_code}" or --fail with exit code)
for the POST using the same credentials (JENKINS_USERNAME/JENKINS_API_TOKEN) and
the "${url}doDelete" target, then check the status: log or print a failure
message and mark the item as not deleted when the status is not a 2xx (or when
curl exits non‑zero), and only print the "Deleting" success line on confirmed
success; ensure non‑2xx responses cause the script to either record the failure
or exit as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9ce3bbb-c253-4fc4-81ae-117fa6108a08
📒 Files selected for processing (8)
hack/ci/git-org-cleanup/bitbucket-repos-cleanup.shhack/ci/git-org-cleanup/gitlab-repos-cleanup.shhack/ci/image-org-cleanup/artifactory-image-cleanup.shhack/ci/image-org-cleanup/nexus-image-cleanup.shhack/ci/image-org-cleanup/quay-image-cleanup.shhack/ci/service-jenkins-cleanup/jenkins-cleanup.shintegration-tests/pipelines/periodic-cleanup-pipeline.yamlintegration-tests/tasks/download-and-execute-cleanup.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- hack/ci/git-org-cleanup/gitlab-repos-cleanup.sh
- hack/ci/git-org-cleanup/bitbucket-repos-cleanup.sh
- integration-tests/tasks/download-and-execute-cleanup.yaml
- integration-tests/pipelines/periodic-cleanup-pipeline.yaml
Comment on lines
+115
to
+120
| # Fetch repository details to get tags (list API doesn't include last_modified) | ||
| repo_detail_url="$QUAY_URL/api/v1/repository/$QUAY_ORGANIZATION/$repo_name" | ||
| repo_detail=$(curl -s -H "$AUTH_HEADER" "$repo_detail_url") | ||
|
|
||
| # Get the most recent tag's last_modified date | ||
| last_modified=$(echo "$repo_detail" | jq -r '[.tags[].last_modified // empty] | map(select(. != null and . != "")) | sort | last // empty') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n hack/ci/image-org-cleanup/quay-image-cleanup.shRepository: redhat-appstudio/tssc-cli
Length of output: 7874
🌐 Web query:
Quay.io API /api/v1/repository namespace repo includeTags tags endpoint
💡 Result:
The Quay.io API endpoint for retrieving repository information including tags is GET /api/v1/repository/{namespace}/{repo}, with the optional query parameter includeTags=true to include tags in the response. Example: curl -X GET -H "Authorization: Bearer <bearer_token>" "https://quay.io/api/v1/repository/{namespace}/{repo}?includeTags=true" This returns repository details and a "tags" object with tag information (name, size, last_modified, manifest_digest, etc.). For a dedicated list of tags (with pagination and filters like onlyActiveTags, limit), use GET /api/v1/repository/{namespace}/{repo}/tag/. Requires repo:read scope. Official base: https://quay.io/api/v1/ (same for Quay Enterprise).
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_quay/3.8/html-single/red_hat_quay_api_guide/index
- 2: https://docs.quay.io/api/
- 3: https://docs.redhat.com/en/documentation/red_hat_quay/3.14/html/red_hat_quay_api_reference/repository
- 4: https://docs.redhat.com/en/documentation/red_hat_quay/3.16/html/red_hat_quay_api_reference/tag
- 5: https://docs.redhat.com/ja/documentation/red_hat_quay/3/html/use_red_hat_quay/viewing-and-modifying-tags-api
🏁 Script executed:
# Test 1: jq behavior with missing .tags field
echo '{"name":"test","description":"test repo"}' | jq -r '[.tags[].last_modified // empty] | map(select(. != null and . != "")) | sort | last // empty' 2>&1
echo "Exit code: $?"
# Test 2: Same test with set -e -o pipefail
bash -c 'set -e -o pipefail; echo "{\"name\":\"test\",\"description\":\"test repo\"}" | jq -r "[.tags[].last_modified // empty] | map(select(. != null and . != \"\")) | sort | last // empty"' 2>&1
echo "Exit code from pipefail test: $?"
# Test 3: With null .tags
echo '{"name":"test","tags":null}' | jq -r '[.tags[].last_modified // empty] | map(select(. != null and . != "")) | sort | last // empty' 2>&1
echo "Exit code with null tags: $?"
# Test 4: With empty .tags array
echo '{"name":"test","tags":[]}' | jq -r '[.tags[].last_modified // empty] | map(select(. != null and . != "")) | sort | last // empty' 2>&1
echo "Exit code with empty tags array: $?"Repository: redhat-appstudio/tssc-cli
Length of output: 350
Add ?includeTags=true to the repository API call on line 116.
The endpoint GET /api/v1/repository/{namespace}/{repo} does not return tags by default; the includeTags=true parameter is required. Without it, the .tags field may be absent or null, causing the jq expression on line 120 to error (Cannot iterate over null) and crash the script under set -o pipefail (line 9).
Change line 116 to:
repo_detail_url="$QUAY_URL/api/v1/repository/$QUAY_ORGANIZATION/$repo_name?includeTags=true"
Alternatively, use the dedicated /tag/ endpoint (GET /api/v1/repository/{namespace}/{repo}/tag/) for tag metadata with pagination and filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh` around lines 115 - 120, The
repo_detail API call is missing the includeTags query so .tags can be null;
update the construction of repo_detail_url (where QUAY_URL, QUAY_ORGANIZATION
and repo_name are combined) to request tags (e.g., append ?includeTags=true) so
repo_detail contains .tags before computing last_modified (used by
last_modified=$(echo "$repo_detail" | jq -r '[.tags[].last_modified // empty] |
...')); alternatively, replace the repo_detail/tag fetch by calling the
repository tag endpoint (/api/v1/repository/{namespace}/{repo}/tag/) and
populate repo_detail or directly derive last_modified from that response.
21e4e52 to
37c09f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh (1)
198-202:⚠️ Potential issue | 🔴 CriticalGuard the indent decrement for unknown classes.
At top level, the warning branch leaves
indentempty.${indent::-6}on Line 202 then raisessubstring expression < 0underset -o errexit, aborting the entire run on the first unhandled Jenkins item type.Suggested change
- indent=${indent::-6} + if ((${`#indent`} >= 6)); then + indent=${indent::-6} + else + indent="" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh` around lines 198 - 202, The else branch that echoes "WARNING: Unhandled class ${class}" may leave indent empty and the subsequent unconditional decrement indent=${indent::-6} causes a "substring expression < 0" error; update the cleanup so you only perform the decrement when indent has at least 6 characters (or is non-empty). Locate the else/fi block that prints the warning and the following indent=${indent::-6} expression and wrap the decrement in a guard (e.g., check [ -n "$indent" ] or ${`#indent`} -ge 6) before modifying the indent variable to avoid the substring error.hack/ci/image-org-cleanup/artifactory-image-cleanup.sh (1)
124-128:⚠️ Potential issue | 🟠 MajorDon't turn parse failures into delete decisions.
Line 125 uses
|| echo "0", so malformedmodified/createdvalues look older than the cutoff and become deletion candidates. Skip the item whendate -dfails instead.Suggested change
- item_modified_time=$(date -d "$item_modified" +%s 2>/dev/null || echo "0") + if ! item_modified_time=$(date -d "$item_modified" +%s 2>/dev/null); then + echo "Skipping item '$item_name': invalid modified timestamp '$item_modified'" >&2 + continue + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 124 - 128, The script currently sets item_modified_time=$(date -d "$item_modified" +%s 2>/dev/null || echo "0") which treats parse failures as epoch 0 and causes deletes; instead detect date parse failure and skip the item: run the date -d call for "$item_modified" and if it fails (non‑zero exit) or returns empty, log/skip the item and continue, otherwise assign the numeric timestamp to item_modified_time and then compare against cutoff_time in the existing if; reference the variables/expressions item_modified, item_modified_time, the date -d invocation, and cutoff_time to locate where to add the failure check and early continue.hack/ci/image-org-cleanup/nexus-image-cleanup.sh (1)
120-124:⚠️ Potential issue | 🟠 MajorSkip unreadable
lastModifiedvalues.Line 121 falls back to
0, which makes a malformed timestamp look older than the cutoff and turns it into a delete candidate. Parse failures should be logged and skipped instead.Suggested change
- last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null || echo "0") + if ! last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null); then + echo "Skipping component '$component_name': invalid lastModified '$last_modified'" >&2 + continue + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh` around lines 120 - 124, The script currently converts last_modified to a Unix timestamp with last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null || echo "0"), which treats parse failures as 0 and may incorrectly mark malformed dates for deletion; change this to detect parse failures (e.g., capture date output to last_modified_time and if empty or non-numeric) log a warning including the component id and the offending last_modified value, and skip that component (continue) instead of setting last_modified_time to 0 so the faulty record is not compared against cutoff_time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/ci/git-org-cleanup/gitlab-repos-cleanup.sh`:
- Around line 75-76: The script currently sets project_count from the
single-page variable projects which only contains up to per_page=200 items, so
counts and logs underreport for groups >200 repos; update the fetching logic
around the API call that builds projects (the block using per_page=200 and the
projects variable) to handle GitLab pagination—either accumulate results across
pages in a loop (incrementing page) until no more items are returned or use the
API response headers (e.g., X-Total or next-page) to fetch all pages,
concatenate into the projects collection, and then compute project_count from
that aggregated projects before echoing; ensure project_count reflects the total
aggregated set rather than a single-page response.
- Around line 88-89: The deletion gate currently only checks age
(last_activity_time < cutoff_time); update it to require both an age check and a
name-pattern match so only intended projects are deleted. Change the conditional
that uses last_activity_time and cutoff_time to also test the repository name
(e.g., project_name) against the configured pattern variable (e.g., name_pattern
or PROJECT_NAME_PATTERN) using a shell regex test ([[ $project_name =~
$name_pattern ]]) and ensure the script validates name_pattern is set before
deletion. This mirrors the GitHub cleanup behavior: proceed with deletion only
when both the name matches the pattern and last_activity_time is older than
cutoff_time.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Around line 119-120: The current jq expression assigned to last_modified reads
and sorts RFC 2822 timestamps as plain strings, which yields incorrect "newest"
selection; update the jq pipeline that builds last_modified from repo_detail to
parse each .tags[].last_modified into epoch seconds using strptime(...) and
mktime (e.g., map tags to objects with an epoch field, sort_by(.epoch) | last)
and then extract the last_modified value, so the sort is numeric by parsed time
rather than lexicographic; modify the expression that sets last_modified (the jq
that references repo_detail and .tags[].last_modified) to use strptime+ mktime
and sort_by(.epoch) | last.
- Line 153: The repository loop uses a here-string which produces a single blank
record when .repositories is empty, causing an empty repo_name and unwanted
detail lookup; change the loop that consumes repos (the while loop that reads
into repo_name from "$(echo "$repos" | jq -c '.repositories[]')") to use process
substitution (e.g., while ...; do ...; done < <(echo "$repos" | jq -c
'.repositories[]')) so no blank record is fed when the array is empty and the
loop becomes a no-op for an empty org.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 185-197: The code only calls item_cleanup when indent equals "
", so nested jobs/folders are never deleted; remove that restrictive indent
check and invoke item_cleanup("$name","$url") after processing both folder and
job/workflow cases (i.e., in the blocks using process_folder and
process_workflow) so each inspected item can be subject to cleanup regardless of
its indent level; keep the verbose echo logic unchanged and ensure you reference
the existing functions/variables item_cleanup, process_folder, process_workflow,
name, url, and indent when making the change.
---
Duplicate comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 124-128: The script currently sets item_modified_time=$(date -d
"$item_modified" +%s 2>/dev/null || echo "0") which treats parse failures as
epoch 0 and causes deletes; instead detect date parse failure and skip the item:
run the date -d call for "$item_modified" and if it fails (non‑zero exit) or
returns empty, log/skip the item and continue, otherwise assign the numeric
timestamp to item_modified_time and then compare against cutoff_time in the
existing if; reference the variables/expressions item_modified,
item_modified_time, the date -d invocation, and cutoff_time to locate where to
add the failure check and early continue.
In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh`:
- Around line 120-124: The script currently converts last_modified to a Unix
timestamp with last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null ||
echo "0"), which treats parse failures as 0 and may incorrectly mark malformed
dates for deletion; change this to detect parse failures (e.g., capture date
output to last_modified_time and if empty or non-numeric) log a warning
including the component id and the offending last_modified value, and skip that
component (continue) instead of setting last_modified_time to 0 so the faulty
record is not compared against cutoff_time.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 198-202: The else branch that echoes "WARNING: Unhandled class
${class}" may leave indent empty and the subsequent unconditional decrement
indent=${indent::-6} causes a "substring expression < 0" error; update the
cleanup so you only perform the decrement when indent has at least 6 characters
(or is non-empty). Locate the else/fi block that prints the warning and the
following indent=${indent::-6} expression and wrap the decrement in a guard
(e.g., check [ -n "$indent" ] or ${`#indent`} -ge 6) before modifying the indent
variable to avoid the substring error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d54a34a-2030-4407-84f5-041f067ccd3f
📒 Files selected for processing (8)
hack/ci/git-org-cleanup/bitbucket-repos-cleanup.shhack/ci/git-org-cleanup/gitlab-repos-cleanup.shhack/ci/image-org-cleanup/artifactory-image-cleanup.shhack/ci/image-org-cleanup/nexus-image-cleanup.shhack/ci/image-org-cleanup/quay-image-cleanup.shhack/ci/service-jenkins-cleanup/jenkins-cleanup.shintegration-tests/pipelines/periodic-cleanup-pipeline.yamlintegration-tests/tasks/download-and-execute-cleanup.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/ci/git-org-cleanup/bitbucket-repos-cleanup.sh
- integration-tests/tasks/download-and-execute-cleanup.yaml
Comment on lines
+119
to
+120
| # Get the most recent tag's last_modified date | ||
| last_modified=$(echo "$repo_detail" | jq -r '[.tags[].last_modified // empty] | map(select(. != null and . != "")) | sort | last // empty') |
There was a problem hiding this comment.
Pick the newest tag by parsed time.
Lines 119-120 sort RFC 2822 last_modified values as plain strings. That can select an older tag as the “latest”, which makes an active repository look stale and eligible for deletion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh` around lines 119 - 120, The
current jq expression assigned to last_modified reads and sorts RFC 2822
timestamps as plain strings, which yields incorrect "newest" selection; update
the jq pipeline that builds last_modified from repo_detail to parse each
.tags[].last_modified into epoch seconds using strptime(...) and mktime (e.g.,
map tags to objects with an epoch field, sort_by(.epoch) | last) and then
extract the last_modified value, so the sort is numeric by parsed time rather
than lexicographic; modify the expression that sets last_modified (the jq that
references repo_detail and .tags[].last_modified) to use strptime+ mktime and
sort_by(.epoch) | last.
Contributor
Author
@jkopriva sure, I'll update gitlab and bitbucket scripts to have regex pattern based on tssc-tests. |
chetna14manku
force-pushed
the
RHTAP-6210
branch
from
37c09f9 to
cbae780
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh (1)
74-74:⚠️ Potential issue | 🟡 Minor
doDeletecurl is missing-Land any status check.The other curl calls in this script use
-Lto follow redirects, but the actual deletion call does not. Jenkins can respond with a 302 redirect ondoDelete(e.g., when behind a reverse proxy or when redirecting back to the parent folder after deletion). Without-L, the request body/method may not reach the final target, and the script cannot distinguish a successful delete from an unauthorized/404 response because the exit status ofcurl -sis only non-zero on transport errors.🔧 Proposed change (combines redirect-follow + HTTP status check)
if [[ "$dry_run" == "false" ]]; then printf "%-10s %-60s %-40s\n" "Deleting" "${name}" "${mod_date}" - curl -s -X POST -u "$JENKINS_USERNAME:$JENKINS_API_TOKEN" "${url}doDelete" + local http_code + http_code=$(curl -s -L -o /dev/null -w "%{http_code}" \ + -X POST -u "$JENKINS_USERNAME:$JENKINS_API_TOKEN" "${url}doDelete") + if [[ "$http_code" -lt 200 || "$http_code" -ge 300 ]]; then + echo "ERROR: Failed to delete '${name}' (HTTP ${http_code})" >&2 + fi elseNote: the missing HTTP status check itself was already raised on a previous revision and still applies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh` at line 74, The curl invocation that posts to "${url}doDelete" is missing -L and any HTTP status check; update the curl call that currently uses curl -s -X POST -u "$JENKINS_USERNAME:$JENKINS_API_TOKEN" "${url}doDelete" to include -L to follow redirects and capture the HTTP response code (e.g., via curl -w '%{http_code}' -o /dev/null or equivalent) and then assert the status is a 2xx success code before treating the delete as successful; ensure the script exits non‑zero or logs an error when the HTTP status indicates failure (non‑2xx) so deletions behind redirects or returning 302/401/404 are detected.
🧹 Nitpick comments (1)
hack/ci/image-org-cleanup/artifactory-image-cleanup.sh (1)
118-154: Guard the processing loop against an empty.resultsarray.When
items.results == [],jq -c '.results[]'produces no output, but the here-string<<< "$(…)"still feeds one empty line intoread -r, so the loop body runs once withitem="".echo "" | jq -r '.name'then fails underpipefail; in a command substitutionerrexitisn't propagated so execution continues with an emptyitem_name(benign today), but it's a fragile pattern. Either skip the loop whenitem_count -eq 0, or switch to process substitution and short-circuit on empty lines.♻️ Suggested refactor
- item_count=$(echo "$items" | jq '.results | length') - echo "Found $item_count items" - - # Process items - while read -r item; do - item_name=$(echo "$item" | jq -r '.name') + item_count=$(echo "$items" | jq '.results | length') + echo "Found $item_count items" + if [[ "$item_count" -eq 0 ]]; then + return 0 + fi + + # Process items + while IFS= read -r item; do + [[ -z "$item" ]] && continue + item_name=$(echo "$item" | jq -r '.name') ... - done <<< "$(echo "$items" | jq -c '.results[]')" + done < <(echo "$items" | jq -c '.results[]')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 118 - 154, The while-read loop can run once with an empty item when items.results is [], causing jq failures; before the loop, compute item_count=$(echo "$items" | jq '.results | length') and if [[ "$item_count" -eq 0 ]]; then skip/return, otherwise iterate; alternatively replace the here-string with process substitution and a short-circuit to avoid an empty input: done < <(echo "$items" | jq -c '.results[]' || true). Apply this check around the existing while read -r item; do ... done block (referencing variables/items, item_count, repo_name_regex, and the delete logic) so the loop never processes a blank item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 20-23: The usage text incorrectly claims ARTIFACTORY_URL has a
hardcoded default; update the help/usage block so it reflects the actual
behavior in artifactory_cleanup(): mention that ARTIFACTORY_URL is optional only
if the file /usr/local/rhtap-cli-install/artifactory-url exists (describe the
file-fallback behavior) and otherwise is required, or alternatively implement
the advertised default URL inside artifactory_cleanup() where ARTIFACTORY_URL is
resolved; reference the ARTIFACTORY_URL variable and the artifactory_cleanup()
function when making the change.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Line 13: The script declares repo_name_regex but never applies it, so add a
regex filter using the existing repo_name variable before the age/cutoff check
in the main processing loop (the block that compares last_modified_time and
cutoff_time); specifically, test [[ "$repo_name" =~ $repo_name_regex ]] and
skip/log repositories that do not match, only proceeding to evaluate age and
perform deletion for repos that match the regex, and update the log lines (the
misleading "Repository name matched regex and is old enough") to reflect the
actual checks performed.
- Around line 43-45: The script currently uses exported environment variables
(QUAY_API_TOKEN, QUAY_ORGANIZATION, QUAY_URL) which unnecessarily leak secrets
to child processes; change the declarations to plain assignments (remove export)
so the variables are available to the script but not inherited by children,
keeping QUAY_API_TOKEN only consumed where the Authorization header is
constructed (in quay_cleanup). Update the assignments that initialize these
variables to use simple variable assignment and confirm any downstream uses
reference the local variables (e.g., in quay_cleanup and curl header
construction) rather than relying on exported env.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Line 91: The curl URL construction can create double slashes because
workflow_url, folder_url and JENKINS_URL may already end with '/', so update the
curl invocations that set build_list and other similar variables to normalize
joins: trim any trailing slash from the base URL variables (workflow_url,
folder_url, JENKINS_URL) before appending "api/json?..." or ensure you always
insert a single '/' (e.g., normalize base="${var%/}" then call
"${base}/api/json..."). Apply this change to the places that set build_list
(uses workflow_url), the folder API call (uses folder_url), and the top-level
Jenkins call (uses JENKINS_URL) so all joins are consistent and avoid double
slashes.
- Around line 107-122: The timestamp read from jq can be "null" or empty and
causes bash arithmetic to misinterpret it; change the jq invocation to not
swallow stderr (remove 2>/dev/null) and validate timestamp before using (( ...
)): after timestamp=$(echo "$build_item" | jq -r '.timestamp'), check that
timestamp is a non-empty numeric string (e.g. [[ "$timestamp" =~ ^[0-9]+$ ]]);
if it is numeric perform the (( timestamp > check_date )) and update last_mod,
otherwise treat the build as having unknown timestamp (set
delete_folder="false", optionally log via verbose and break/continue to avoid
treating it as old) so jobs without valid timestamps are not considered
deletable.
---
Duplicate comments:
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Line 74: The curl invocation that posts to "${url}doDelete" is missing -L and
any HTTP status check; update the curl call that currently uses curl -s -X POST
-u "$JENKINS_USERNAME:$JENKINS_API_TOKEN" "${url}doDelete" to include -L to
follow redirects and capture the HTTP response code (e.g., via curl -w
'%{http_code}' -o /dev/null or equivalent) and then assert the status is a 2xx
success code before treating the delete as successful; ensure the script exits
non‑zero or logs an error when the HTTP status indicates failure (non‑2xx) so
deletions behind redirects or returning 302/401/404 are detected.
---
Nitpick comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 118-154: The while-read loop can run once with an empty item when
items.results is [], causing jq failures; before the loop, compute
item_count=$(echo "$items" | jq '.results | length') and if [[ "$item_count" -eq
0 ]]; then skip/return, otherwise iterate; alternatively replace the here-string
with process substitution and a short-circuit to avoid an empty input: done <
<(echo "$items" | jq -c '.results[]' || true). Apply this check around the
existing while read -r item; do ... done block (referencing variables/items,
item_count, repo_name_regex, and the delete logic) so the loop never processes a
blank item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ff7b627-7ebc-4328-82cf-14e02e2ec1b6
📒 Files selected for processing (7)
hack/ci/git-org-cleanup/gitlab-repos-cleanup.shhack/ci/image-org-cleanup/artifactory-image-cleanup.shhack/ci/image-org-cleanup/nexus-image-cleanup.shhack/ci/image-org-cleanup/quay-image-cleanup.shhack/ci/service-jenkins-cleanup/jenkins-cleanup.shintegration-tests/pipelines/periodic-cleanup-pipeline.yamlintegration-tests/tasks/download-and-execute-cleanup.yaml
✅ Files skipped from review due to trivial changes (1)
- hack/ci/image-org-cleanup/nexus-image-cleanup.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- hack/ci/git-org-cleanup/gitlab-repos-cleanup.sh
- integration-tests/tasks/download-and-execute-cleanup.yaml
- integration-tests/pipelines/periodic-cleanup-pipeline.yaml
Comment on lines
+107
to
+122
| while read -r build_item; do | ||
| local timestamp | ||
| timestamp=$(echo "$build_item" | jq -r '.timestamp' 2>/dev/null) | ||
|
|
||
| if (( timestamp > check_date )); then | ||
| delete_folder="false" | ||
| if [[ "$verbose" == "true" ]]; then | ||
| echo "${indent}Skipping - Build has recently run" | ||
| fi | ||
| break | ||
| fi | ||
|
|
||
| if (( timestamp > last_mod )); then | ||
| last_mod=$timestamp | ||
| fi | ||
| done < <(echo "$build_list" | jq -c '.[]') |
There was a problem hiding this comment.
Arithmetic comparison can silently misbehave when timestamp is not numeric.
jq -r '.timestamp' 2>/dev/null returns the string null when the build lacks a timestamp field, and the 2>/dev/null also hides jq errors (e.g., invalid JSON), leaving timestamp as an empty string. Bash's (( timestamp > check_date )) then treats the value as a variable name, coerces it to 0, and the comparison silently yields "old build", so last_mod stays at 0 and the job can become eligible for deletion even though its true timestamp is unknown.
Consider validating the value before comparing, and drop the stderr redirection so real failures surface:
🛡️ Defensive guard
- local timestamp
- timestamp=$(echo "$build_item" | jq -r '.timestamp' 2>/dev/null)
-
- if (( timestamp > check_date )); then
+ local timestamp
+ timestamp=$(echo "$build_item" | jq -r '.timestamp // empty')
+ if [[ -z "$timestamp" || ! "$timestamp" =~ ^[0-9]+$ ]]; then
+ if [[ "$verbose" == "true" ]]; then
+ echo "${indent}Skipping build with missing/invalid timestamp"
+ fi
+ delete_folder="false"
+ break
+ fi
+
+ if (( timestamp > check_date )); then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh` around lines 107 - 122,
The timestamp read from jq can be "null" or empty and causes bash arithmetic to
misinterpret it; change the jq invocation to not swallow stderr (remove
2>/dev/null) and validate timestamp before using (( ... )): after
timestamp=$(echo "$build_item" | jq -r '.timestamp'), check that timestamp is a
non-empty numeric string (e.g. [[ "$timestamp" =~ ^[0-9]+$ ]]); if it is numeric
perform the (( timestamp > check_date )) and update last_mod, otherwise treat
the build as having unknown timestamp (set delete_folder="false", optionally log
via verbose and break/continue to avoid treating it as old) so jobs without
valid timestamps are not considered deletable.
cbae780 to
1b35b72
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
hack/ci/image-org-cleanup/nexus-image-cleanup.sh (2)
128-132:⚠️ Potential issue | 🟠 MajorUnparseable
lastModifiedis still treated as epoch 0 → forced deletion.Line 129 falls back to
echo "0"whendate -d "$last_modified"fails, so any component with a malformed or unexpected timestamp format is unconditionally older thancutoff_timeand gets deleted at Line 132–146 without any warning. Detect the parse failure and skip (or log and skip) the component instead of defaulting to0.🛡️ Suggested fix
- # Convert last_modified to Unix timestamp (Nexus uses ISO 8601 format) - last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null || echo "0") + # Convert last_modified to Unix timestamp (Nexus uses ISO 8601 format) + if ! last_modified_time=$(date -d "$last_modified" +%s 2>/dev/null); then + echo "Skipping component '$component_name': unparseable lastModified '$last_modified'" >&2 + continue + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh` around lines 128 - 132, The script currently treats unparseable lastModified timestamps as epoch 0 by falling back to echo "0" for last_modified_time, which forces deletion; change the parsing logic around last_modified and last_modified_time so you detect date -d failure (no silent fallback), log or emit a warning referencing the component and the raw last_modified value, and skip processing that component (continue) instead of assigning 0; update the condition that compares last_modified_time and cutoff_time to only run the deletion block when last_modified_time was successfully parsed and is less than cutoff_time.
72-79:⚠️ Potential issue | 🟠 MajorMissing Nexus pagination — only the first page is cleaned.
/service/rest/v1/componentsis paginated (default 100 items) and returns acontinuationTokenwhen more results remain. The fetch at Line 78 runs once and the loop at Line 113 iterates only that first page, so repositories with more than one page of components will never have their older entries cleaned. Wrap the fetch in a loop that accumulates.itemsand re-requests with&continuationToken=<token>until the token isnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh` around lines 72 - 79, The current fetch only retrieves the first paginated page from api_url and assigns components from tmp_file; modify the logic around api_url/tmp_file/http_status/components to loop requests until the Nexus JSON response's continuationToken is null, accumulating all .items across pages; on each iteration call the same curl (with -u "$AUTH_CREDS") but append &continuationToken=<token> when a token is returned, merge the .items into a single components collection, and only then continue to the existing cleanup loop so all pages are cleaned.hack/ci/image-org-cleanup/quay-image-cleanup.sh (1)
43-45:⚠️ Potential issue | 🔴 CriticalSeveral previously-raised issues are still present in this revision.
Please confirm whether the following are intentionally deferred or still outstanding — none of them appear to have been addressed in the current file:
- Line 43–45:
QUAY_API_TOKEN(andQUAY_ORGANIZATION/QUAY_URL) are stillexported, leaking the token into every childcurl. Plain assignment is sufficient since the token is only consumed via$AUTH_HEADERconstructed locally.- Line 123:
repo_detail_urlis still built without?includeTags=true. TheGET /api/v1/repository/{ns}/{repo}endpoint does not include tags by default, so.tagsmay be missing; combined withset -o pipefail, the jq on Line 127 can fail hard. Either append?includeTags=trueor switch to/tag/endpoint.- Line 127:
[.tags[].last_modified] | sort | laststill sorts RFC 2822 strings lexicographically, which does not yield the newest tag (e.g.,"Fri, …">"Wed, …"by string order). This can mark an active repo as stale. Convert viastrptime/mktimebefore sorting, or do the Unix-epoch conversion inside jq.- Line 136–137:
date -d "$last_modified" +%s 2>/dev/null || echo "0"still falls back to epoch0on parse failure, which is always older thancutoff_timeand would cause deletion. Skip the repo on parse failure instead.- Line 160:
done <<< "$(echo "$repos" | jq -c '.repositories[]')"still feeds one blank record to the loop when.repositoriesis empty; usedone < <(echo "$repos" | jq -c '.repositories[]')so an empty org is a true no-op.Also applies to: 123-123, 127-127, 136-137, 160-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh` around lines 43 - 45, Do not export sensitive tokens and related vars globally: change QUAY_API_TOKEN/QUAY_ORGANIZATION/QUAY_URL from exported environment variables to plain assignments (they are only used to build AUTH_HEADER locally). Update repo_detail_url construction in the code that builds repo_detail_url to include ?includeTags=true (or switch to the /tag/ endpoint) so .tags is returned. In the jq call that computes last_modified (the expression using [.tags[].last_modified] | sort | last), parse tag dates to epoch with strptime/mktime (or convert to Unix epoch inside jq) before sorting so newest tag is determined correctly. Replace the fallback behavior that uses date -d "$last_modified" +%s || echo "0" (which always yields 0 on parse failure) with logic that skips the repo if date parsing fails instead of treating it as epoch 0; reference last_modified and cutoff_time when adding the skip. Finally, change the loop feeding jq output from done <<< "$(echo "$repos" | jq -c '.repositories[]')" to a process-substitution form (done < <(echo "$repos" | jq -c '.repositories[]')) so an empty .repositories array results in no iterations rather than a single blank record.hack/ci/image-org-cleanup/artifactory-image-cleanup.sh (3)
20-23:⚠️ Potential issue | 🟡 MinorUsage text still advertises an
ARTIFACTORY_URLdefault the code does not implement.Line 21 documents
https://artifactory-ui-artifactory-jcr2.apps.rosa.rhtap-services.xmdt.p3.openshiftapps.comas a default, butartifactory_cleanupat line 43 only falls back to reading/usr/local/rhtap-cli-install/artifactory-url. A user running--helpoutside CI will hitError: Required environment variables are not setdespite following the documented default. Either wire the documented default into line 43 or update usage to describe the credential-file fallback.📝 Proposed doc fix (if the file fallback is the intended behavior)
- ARTIFACTORY_URL - Artifactory server URL (default: https://artifactory-ui-artifactory-jcr2.apps.rosa.rhtap-services.xmdt.p3.openshiftapps.com) + ARTIFACTORY_URL - Artifactory server URL + (falls back to /usr/local/rhtap-cli-install/artifactory-url when unset) ARTIFACTORY_REPOSITORY - Artifactory repository name (default: rhtap) - ARTIFACTORY_API_TOKEN - Artifactory API token + ARTIFACTORY_API_TOKEN - Artifactory API token + (falls back to /usr/local/rhtap-cli-install/artifactory-token when unset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 20 - 23, The usage text claims ARTIFACTORY_URL has a specific default but the artifactory_cleanup function only falls back to reading /usr/local/rhtap-cli-install/artifactory-url; update one of them: either modify artifactory_cleanup to use the documented hardcoded default URL when ARTIFACTORY_URL is unset (ensure the function reads ARTIFACTORY_URL || that default) or change the usage/help text to state the actual fallback behavior (mention the credential-file /usr/local/rhtap-cli-install/artifactory-url) so the help output matches the implemented behavior.
69-72:⚠️ Potential issue | 🟠 MajorAQL query has a hard
limit(200)with no pagination, and the comment is stale.Line 70 still says "limited to 100" while line 71 sets
.limit(200). More importantly, for repositories with more than 200 folders matching the criteria, items beyond 200 are silently ignored and never cleaned up. Add an.offset(N)loop that advances until a page returns zero rows, and fix the comment.♻️ Sketch of pagination
- # Use AQL (Artifactory Query Language) to find items - # This finds all Docker images in the repository, limited to 100 - aql_query='items.find({"repo":"'"$ARTIFACTORY_REPOSITORY"'","type":"folder","depth":1}).include("name","created","modified").limit(200)' - api_url="$ARTIFACTORY_URL/artifactory/api/search/aql" + # Use AQL (Artifactory Query Language) to find items. + # Page through results to avoid losing anything when the repo has > page_size folders. + page_size=200 + offset=0 + api_url="$ARTIFACTORY_URL/artifactory/api/search/aql" + while : ; do + aql_query='items.find({"repo":"'"$ARTIFACTORY_REPOSITORY"'","type":"folder","depth":1}).include("name","created","modified").offset('"$offset"').limit('"$page_size"')' + # ...issue request, process page, break when .results length < page_size + offset=$((offset + page_size)) + done
132-136:⚠️ Potential issue | 🔴 CriticalFallback to
0on timestamp parse failure still turns malformed dates into delete candidates.Line 133 falls back to
0whendate -d "$item_modified"fails, and line 136 then sees epoch (1970) as older than cutoff, so any item whosemodified/createdvalue Artifactory returns in an unparseable form would be deleted. Treat parse failure as "skip" rather than "ancient".🛡️ Suggested fix
- # Convert modified time to Unix timestamp (Artifactory uses ISO 8601 format) - item_modified_time=$(date -d "$item_modified" +%s 2>/dev/null || echo "0") - - # Image name matched regex and is old enough - if [[ $item_modified_time -lt $cutoff_time ]]; then + # Convert modified time to Unix timestamp (Artifactory uses ISO 8601 format) + if ! item_modified_time=$(date -d "$item_modified" +%s 2>/dev/null); then + echo "Skipping item '$item_name': invalid modified timestamp '$item_modified'" >&2 + continue + fi + + # Image name matched regex and is old enough + if [[ $item_modified_time -lt $cutoff_time ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 132 - 136, The fallback to "0" on timestamp parse failure makes malformed dates appear ancient and eligible for deletion; change the logic around item_modified_time/item_modified so that you attempt to parse item_modified into a Unix timestamp and if the parse fails (non-numeric or empty) you skip this item (e.g., continue the loop) instead of assigning 0. Locate the parsing expression that sets item_modified_time and replace it with a parse-then-check flow that validates the parse result before comparing to cutoff_time to avoid deleting items with unparseable dates.
🧹 Nitpick comments (2)
hack/ci/image-org-cleanup/quay-image-cleanup.sh (1)
73-76: Optional: add an explicitcurltimeout andtrapthe tempfile.
curl -shere has no--max-time/--connect-timeout, so a slow/hung Quay endpoint can stall the periodic pipeline. Also, if any command betweenmktemp(Line 73) andrm -f(Line 76) fails underset -e, the temp file leaks. Minor, but cheap hardening for a pipeline script.Suggested change
- tmp_file=$(mktemp) - http_status=$(curl -s -o "$tmp_file" -w "%{http_code}" -H "$AUTH_HEADER" "$api_url") - repos=$(cat "$tmp_file") - rm -f "$tmp_file" + tmp_file=$(mktemp) + trap 'rm -f "$tmp_file"' RETURN + http_status=$(curl -sS --connect-timeout 10 --max-time 60 \ + -o "$tmp_file" -w "%{http_code}" -H "$AUTH_HEADER" "$api_url") + repos=$(cat "$tmp_file")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh` around lines 73 - 76, The curl call that sets tmp_file via mktemp and reads repos can hang and leak the temp file if a command fails; update the curl invocation (the line that sets http_status using curl with AUTH_HEADER and api_url) to include sensible timeouts (e.g., --connect-timeout and --max-time) and add a trap immediately after tmp_file is created to rm -f "$tmp_file" on EXIT (so the tempfile is always cleaned up even if set -e triggers); ensure you still capture http_status and repos the same way and reference tmp_file, http_status, AUTH_HEADER and api_url when making the change.hack/ci/image-org-cleanup/artifactory-image-cleanup.sh (1)
114-118: Skip the loop entirely when there are zero results.
done <<< "$(echo "$items" | jq -c '.results[]')"produces one empty iteration when.resultsis empty (here-strings always append a newline). Today this is benign because the regex on an emptyitem_namefails, but it's noisy and a latent foot-gun if the loop ever gains side effects before the regex check. Short-circuiting onitem_countis cheap and clearer.♻️ Suggested tweak
item_count=$(echo "$items" | jq '.results | length') echo "Found $item_count items" + + if [[ "$item_count" -eq 0 ]]; then + return 0 + fiAlso applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh` around lines 114 - 118, The loop that reads items via done <<< "$(echo "$items" | jq -c '.results[]')" runs one empty iteration when .results is empty; fix this by short-circuiting: after computing item_count from items (variable item_count), check if item_count is zero and skip the processing loop (and mirror the same check around the other loop at the second occurrence). Ensure the check runs before entering the while read -r item loop so no iteration or side-effects occur when there are no results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Line 12: Validate the DAYS environment variable (used in the cutoff_time
calculation) to ensure it is a non-negative integer before it is used; add a
guard immediately after the DAYS="${DAYS:-14}" assignment that checks DAYS
matches a digits-only regex (e.g. ^[0-9]+$) and that it is not negative, and if
the check fails either set DAYS to the safe default (14) or exit with an error
message; this prevents arithmetic errors and avoids computing a future
cutoff_time in the cutoff_time=$((now - DAYS * 24 * 60 * 60)) calculation.
- Around line 69-76: The current fetch only retrieves the first page from
api_url and assigns it to repos; change the logic in the repository fetch block
to loop over pages using the response's next_page token: perform the curl
request repeatedly (reusing tmp_file and http_status as needed), parse the JSON
response to extract both repositories and the next_page token, append each
page's repository list into a single accumulator (e.g., append to repos variable
or a temp file) and set api_url to
"$QUAY_URL/api/v1/repository?namespace=$QUAY_ORGANIZATION&next_page=<token>" (or
include next_page param only when token exists) until next_page is null/empty,
then continue with the rest of the script using the accumulated repos; ensure
error handling of http_status remains in place for each request.
In `@hack/ci/service-jenkins-cleanup/jenkins-cleanup.sh`:
- Around line 245-286: Change the default for the dry-run flag to
safe-by-default: set the variable dry_run to "true" in parse_args so the script
will not perform deletions unless the user explicitly passes --no-dry-run (or a
new --execute flag if you prefer); update any help text or examples that mention
--no-dry-run to reflect that it is required to actually delete, and ensure the
case branch that handles --no-dry-run sets dry_run="false" (leave parse_args,
the dry_run variable, and the --no-dry-run handling intact so callers must opt
in).
- Around line 27-43: The usage docs in jenkins-cleanup.sh are inconsistent: the
examples reference a --no-dry-run flag but the "Optional arguments" help block
doesn't list it; update the help text in the top-of-file usage block to include
the --no-dry-run option (or remove the example reference) and explicitly state
the default behavior for dry-run so users know whether actions are simulated or
executed; locate the help text and the argument parser implementation in
parse_args to ensure the flag name and description match exactly.
In `@integration-tests/tasks/download-and-execute-cleanup.yaml`:
- Around line 48-58: The credential-extraction block for
SCRIPT_NAME=="delete-rosa-clusters.sh" should stop silently swallowing jq
failures and "null" values: first verify CREDENTIALS_FILE exists and is
readable, then assign each value using jq -er (not -r) into shell variables
(e.g., ROSA_TOKEN_RAW, AWS_ACCESS_KEY_ID_RAW, AWS_SECRET_ACCESS_KEY_RAW,
AWS_SUBNET_IDS_RAW, AWS_DEFAULT_REGION_RAW) so set -e will catch jq failures,
and only then export the validated variables (ROSA_TOKEN, AWS_ACCESS_KEY_ID,
AWS_SECRET_ACCESS_KEY, AWS_SUBNET_IDS, AWS_DEFAULT_REGION); if any jq invocation
fails, print a clear error mentioning CREDENTIALS_FILE and the missing key and
exit non-zero to avoid propagating "null" strings into delete-rosa-clusters.sh.
---
Duplicate comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 20-23: The usage text claims ARTIFACTORY_URL has a specific
default but the artifactory_cleanup function only falls back to reading
/usr/local/rhtap-cli-install/artifactory-url; update one of them: either modify
artifactory_cleanup to use the documented hardcoded default URL when
ARTIFACTORY_URL is unset (ensure the function reads ARTIFACTORY_URL || that
default) or change the usage/help text to state the actual fallback behavior
(mention the credential-file /usr/local/rhtap-cli-install/artifactory-url) so
the help output matches the implemented behavior.
- Around line 132-136: The fallback to "0" on timestamp parse failure makes
malformed dates appear ancient and eligible for deletion; change the logic
around item_modified_time/item_modified so that you attempt to parse
item_modified into a Unix timestamp and if the parse fails (non-numeric or
empty) you skip this item (e.g., continue the loop) instead of assigning 0.
Locate the parsing expression that sets item_modified_time and replace it with a
parse-then-check flow that validates the parse result before comparing to
cutoff_time to avoid deleting items with unparseable dates.
In `@hack/ci/image-org-cleanup/nexus-image-cleanup.sh`:
- Around line 128-132: The script currently treats unparseable lastModified
timestamps as epoch 0 by falling back to echo "0" for last_modified_time, which
forces deletion; change the parsing logic around last_modified and
last_modified_time so you detect date -d failure (no silent fallback), log or
emit a warning referencing the component and the raw last_modified value, and
skip processing that component (continue) instead of assigning 0; update the
condition that compares last_modified_time and cutoff_time to only run the
deletion block when last_modified_time was successfully parsed and is less than
cutoff_time.
- Around line 72-79: The current fetch only retrieves the first paginated page
from api_url and assigns components from tmp_file; modify the logic around
api_url/tmp_file/http_status/components to loop requests until the Nexus JSON
response's continuationToken is null, accumulating all .items across pages; on
each iteration call the same curl (with -u "$AUTH_CREDS") but append
&continuationToken=<token> when a token is returned, merge the .items into a
single components collection, and only then continue to the existing cleanup
loop so all pages are cleaned.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Around line 43-45: Do not export sensitive tokens and related vars globally:
change QUAY_API_TOKEN/QUAY_ORGANIZATION/QUAY_URL from exported environment
variables to plain assignments (they are only used to build AUTH_HEADER
locally). Update repo_detail_url construction in the code that builds
repo_detail_url to include ?includeTags=true (or switch to the /tag/ endpoint)
so .tags is returned. In the jq call that computes last_modified (the expression
using [.tags[].last_modified] | sort | last), parse tag dates to epoch with
strptime/mktime (or convert to Unix epoch inside jq) before sorting so newest
tag is determined correctly. Replace the fallback behavior that uses date -d
"$last_modified" +%s || echo "0" (which always yields 0 on parse failure) with
logic that skips the repo if date parsing fails instead of treating it as epoch
0; reference last_modified and cutoff_time when adding the skip. Finally, change
the loop feeding jq output from done <<< "$(echo "$repos" | jq -c
'.repositories[]')" to a process-substitution form (done < <(echo "$repos" | jq
-c '.repositories[]')) so an empty .repositories array results in no iterations
rather than a single blank record.
---
Nitpick comments:
In `@hack/ci/image-org-cleanup/artifactory-image-cleanup.sh`:
- Around line 114-118: The loop that reads items via done <<< "$(echo "$items" |
jq -c '.results[]')" runs one empty iteration when .results is empty; fix this
by short-circuiting: after computing item_count from items (variable
item_count), check if item_count is zero and skip the processing loop (and
mirror the same check around the other loop at the second occurrence). Ensure
the check runs before entering the while read -r item loop so no iteration or
side-effects occur when there are no results.
In `@hack/ci/image-org-cleanup/quay-image-cleanup.sh`:
- Around line 73-76: The curl call that sets tmp_file via mktemp and reads repos
can hang and leak the temp file if a command fails; update the curl invocation
(the line that sets http_status using curl with AUTH_HEADER and api_url) to
include sensible timeouts (e.g., --connect-timeout and --max-time) and add a
trap immediately after tmp_file is created to rm -f "$tmp_file" on EXIT (so the
tempfile is always cleaned up even if set -e triggers); ensure you still capture
http_status and repos the same way and reference tmp_file, http_status,
AUTH_HEADER and api_url when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef3b588e-5ee7-4e54-8c35-571e530d9b15
📒 Files selected for processing (9)
hack/ci/git-org-cleanup/bitbucket-repos-cleanup.shhack/ci/git-org-cleanup/github-repos-cleanup.shhack/ci/git-org-cleanup/gitlab-repos-cleanup.shhack/ci/image-org-cleanup/artifactory-image-cleanup.shhack/ci/image-org-cleanup/nexus-image-cleanup.shhack/ci/image-org-cleanup/quay-image-cleanup.shhack/ci/service-jenkins-cleanup/jenkins-cleanup.shintegration-tests/pipelines/periodic-cleanup-pipeline.yamlintegration-tests/tasks/download-and-execute-cleanup.yaml
✅ Files skipped from review due to trivial changes (2)
- hack/ci/git-org-cleanup/gitlab-repos-cleanup.sh
- integration-tests/pipelines/periodic-cleanup-pipeline.yaml
chetna14manku
force-pushed
the
RHTAP-6210
branch
from
1b35b72 to
93305b4
Compare
Also move rosa & jenkins cleanup in pipeline
chetna14manku
force-pushed
the
RHTAP-6210
branch
from
93305b4 to
41a9fae
Compare
|
Contributor
Author
|
/retest |
5 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Example konflux run: https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/rhtap-shared-team-tenant/applications/tssc-cli-periodic/pipelineruns/periodic-cleanup-pipeline-run-q92rb
Summary by CodeRabbit
New Features
Improvements
Chores