ROX-35081: Add weekly ACS instance report pipeline - #2752
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the ChangesACS instance reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant GenerateReport
participant FleetManager
participant SlackNotification
Pipeline->>GenerateReport: pass image and pipeline-run metadata
GenerateReport->>FleetManager: run authenticated central report
FleetManager-->>GenerateReport: return output and exit code
GenerateReport->>SlackNotification: send results.message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
kovayur
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.tekton/acs-instance-report-cron.yaml (1)
123-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBuild step memory limit may be too low for a full Go build.
make fleet-managercompiles a large Go module; a 512Mi memory limit can trigger OOM kills during compilation/linking, causing intermittent cron failures. Consider raising the memory limit (e.g., 1–2Gi) and validating against a real run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.tekton/acs-instance-report-cron.yaml around lines 123 - 129, Increase the build step memory limit in the computeResources block used by make fleet-manager from 512Mi to an appropriate value such as 1Gi–2Gi, while retaining the existing request unless needed; validate the cron job with a real full Go build to confirm it no longer encounters OOM failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.tekton/acs-instance-report-cron.yaml:
- Line 121: Update the printf command in the pipeline report step to use a
format that interprets escaped newline sequences, such as %b, so the separators
in MESSAGE and PIPELINE_LINK render as actual blank lines rather than literal \n
text.
---
Nitpick comments:
In @.tekton/acs-instance-report-cron.yaml:
- Around line 123-129: Increase the build step memory limit in the
computeResources block used by make fleet-manager from 512Mi to an appropriate
value such as 1Gi–2Gi, while retaining the existing request unless needed;
validate the cron job with a real full Go build to confirm it no longer
encounters OOM failures.
🪄 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: Enterprise
Run ID: 93654d42-0add-49d7-bc07-f89f1b7147bd
📒 Files selected for processing (13)
.tekton/acs-instance-report-cron.yamlcmd/acsfleetctl/main.gocmd/fleet-manager/main.gointernal/central/pkg/api/admin/private/api/openapi.yamlinternal/central/pkg/api/admin/private/model_central.gointernal/central/pkg/cmd/admin/centrals/cmd.gointernal/central/pkg/cmd/admin/centrals/list.gointernal/central/pkg/cmd/admin/centrals/report.gointernal/central/pkg/cmd/admin/centrals/report_test.gointernal/central/pkg/cmd/admin/cmd.gointernal/central/pkg/cmd/fleetmanagerclient/client.gointernal/central/pkg/presenters/admin_central.goopenapi/fleet-manager-private-admin.yaml
💤 Files with no reviewable changes (1)
- cmd/acsfleetctl/main.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.tekton/acs-instance-report-pipeline.yaml:
- Around line 103-107: Replace the error-message assignment in the report
formatting logic with printf -v so \n becomes actual newlines while preserving
EXIT_CODE and REPORT values; also update the truncation-note assignment at
.tekton/acs-instance-report-pipeline.yaml lines 114-119 to use printf -v and
produce real line breaks.
🪄 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: Enterprise
Run ID: 7902d30f-3a11-468e-b2c2-e8eeec7925a6
📒 Files selected for processing (1)
.tekton/acs-instance-report-pipeline.yaml
|
/konflux-retest |
kovayur
changed the title
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleet-manager/main.go (1)
52-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrevent duplicate error messages on CLI misuse.
By default, Cobra prints command execution errors (such as unknown flags or invalid commands) to
stderrbefore returning them. Becauserun()wraps and returns this error, andmain()prints it again, the user will see the error twice.Consider configuring the root command with
SilenceErrors: true(whererootCmdis defined) to suppress Cobra's automatic error printing, leaving the printing entirely tomain().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet-manager/main.go` around lines 52 - 63, Configure the Cobra root command at its definition with SilenceErrors enabled so command execution failures are not printed automatically. Keep run() returning the wrapped Execute error and let main() remain the sole place that prints CLI errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/fleet-manager/main.go`:
- Around line 52-63: Configure the Cobra root command at its definition with
SilenceErrors enabled so command execution failures are not printed
automatically. Keep run() returning the wrapped Execute error and let main()
remain the sole place that prints CLI errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e8bfdae9-43f5-4912-9ff1-c9f33b689bba
📒 Files selected for processing (4)
.tekton/acs-instance-report-pipeline.yamlcmd/fleet-manager/main.gointernal/central/pkg/cmd/admin/cmd.gointernal/central/pkg/cmd/fleetmanagerclient/client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/central/pkg/cmd/fleetmanagerclient/client.go
- .tekton/acs-instance-report-pipeline.yaml
kovayur
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 45 seconds. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.tekton/acs-instance-report-pipeline.yaml:
- Around line 38-41: Update the fleet-manager-image parameter default in the
pipeline to reference an immutable image digest instead of the mutable :latest
tag. Preserve the existing registry image repository and ensure the digest
identifies the reviewed fleet-manager image.
- Around line 109-118: The truncation logic around MESSAGE, SUFFIX, and CUT must
enforce MAX_BYTES using UTF-8 byte lengths rather than character counts or
offsets. Replace ${`#MESSAGE`}, ${`#SUFFIX`}, and character-based substring
truncation with UTF-8-safe byte-aware truncation, while preserving complete
valid characters and ensuring the final message including SUFFIX never exceeds
MAX_BYTES.
In `@internal/central/pkg/cmd/admin/centrals/report.go`:
- Around line 103-107: Update the region classification around the c.Region
check to include Canadian regions such as ca-central-1 alongside us- regions,
while preserving the existing seven-day CreatedAt filtering and naInstances
append behavior.
- Around line 62-71: Update the pagination loop in the centrals-fetching
function around api.GetCentrals so it detects when list.Items is empty and
terminates instead of requesting further pages. Preserve the existing
total-count termination and fetch error behavior, using an appropriate error
return or break for the empty-page case.
In `@internal/central/pkg/cmd/admin/cmd.go`:
- Around line 34-38: Replace the panic-based impl.OptionFromEnv() call in the
command’s auth initialization with an error-returning environment parser, and
propagate its error through the Cobra command instead of allowing a panic.
Preserve the existing “creating auth” stderr message and exit behavior for
returned errors, while ensuring malformed environment configuration reaches that
handler without a stack trace.
🪄 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: Enterprise
Run ID: 255db1f4-8de6-4ae0-9548-270fb42b017e
📒 Files selected for processing (13)
.tekton/acs-instance-report-pipeline.yamlcmd/acsfleetctl/main.gocmd/fleet-manager/main.gointernal/central/pkg/api/admin/private/api/openapi.yamlinternal/central/pkg/api/admin/private/model_central.gointernal/central/pkg/cmd/admin/centrals/cmd.gointernal/central/pkg/cmd/admin/centrals/list.gointernal/central/pkg/cmd/admin/centrals/report.gointernal/central/pkg/cmd/admin/centrals/report_test.gointernal/central/pkg/cmd/admin/cmd.gointernal/central/pkg/cmd/fleetmanagerclient/client.gointernal/central/pkg/presenters/admin_central.goopenapi/fleet-manager-private-admin.yaml
💤 Files with no reviewable changes (1)
- cmd/acsfleetctl/main.go
| - name: fleet-manager-image | ||
| type: string | ||
| description: Pre-built fleet-manager container image reference | ||
| default: "quay.io/redhat-services-prod/acscs-rhacs-tenant/acscs-main/acs-fleet-manager:latest" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the credential-bearing workload image by digest.
The default :latest tag is mutable, and this image receives the RHSSO client secret. Pin an immutable digest so registry tag movement cannot execute unreviewed code with these credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.tekton/acs-instance-report-pipeline.yaml around lines 38 - 41, Update the
fleet-manager-image parameter default in the pipeline to reference an immutable
image digest instead of the mutable :latest tag. Preserve the existing registry
image repository and ensure the digest identifies the reviewed fleet-manager
image.
There was a problem hiding this comment.
To achieve this, I need the binary changes to be merged first. Therefore, I have split the PR into two parts: this one and #2764.
There was a problem hiding this comment.
@kovayur, understood. The digest cannot identify the new binary until PR #2764 produces the reviewed image.
The finding remains valid for this pipeline. Please pin fleet-manager-image to the resulting immutable digest in PR #2764.
You are interacting with an AI system.
| if strings.HasPrefix(c.Region, "us-") { | ||
| if !c.CreatedAt.Before(sevenDaysAgo) { | ||
| naInstances = append(naInstances, *c) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include Canadian regions in North America.
The us- check excludes regions such as ca-central-1, so Canadian instances are missing from the North America section.
- if strings.HasPrefix(c.Region, "us-") {
+ if strings.HasPrefix(c.Region, "us-") || strings.HasPrefix(c.Region, "ca-") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.HasPrefix(c.Region, "us-") { | |
| if !c.CreatedAt.Before(sevenDaysAgo) { | |
| naInstances = append(naInstances, *c) | |
| } | |
| } | |
| if strings.HasPrefix(c.Region, "us-") || strings.HasPrefix(c.Region, "ca-") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/central/pkg/cmd/admin/centrals/report.go` around lines 103 - 107,
Update the region classification around the c.Region check to include Canadian
regions such as ca-central-1 alongside us- regions, while preserving the
existing seven-day CreatedAt filtering and naInstances append behavior.
| auth, err := impl.NewAuth(ctx, authType, impl.OptionFromEnv()) | ||
| if err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "creating auth: %v\n", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Return environment parsing errors instead of panicking.
impl.OptionFromEnv() uses utils.Must, so malformed environment configuration panics before this error handler and can still print a stack trace. Introduce an error-returning environment parser and propagate that error through Cobra.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/central/pkg/cmd/admin/cmd.go` around lines 34 - 38, Replace the
panic-based impl.OptionFromEnv() call in the command’s auth initialization with
an error-returning environment parser, and propagate its error through the Cobra
command instead of allowing a panic. Preserve the existing “creating auth”
stderr message and exit behavior for returned errors, while ensuring malformed
environment configuration reaches that handler without a stack trace.
Add a Tekton pipeline that runs `fleet-manager admin central report` and posts the result to Slack via webhook. Triggered by a CronJob in konflux-release-data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
da1249f to
f847edd
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kovayur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The Tekton result size limit is byte-based, but the script was using
${#VAR} and ${VAR:0:N} which count characters. With multi-byte UTF-8
content (e.g. organisation names), the result could exceed the limit.
Switch to wc -c and head -c for byte-accurate enforcement.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
PR summary
Adds a weekly ACS instance report pipeline to ACS Fleet Manager.
Comments summary
The following was generated by
@coderabbitaiand may be updated automatically.Summary
Adds a Tekton pipeline that runs the weekly ACS Fleet Manager instance report.
The pipeline:
fleet-manager admin central reportwith RHSSO authentication.acs-instance-reportwebhook.Checklist (Definition of Done)
Test manual
acs-instance-reportpipeline with valid Fleet Manager, Konflux, RHSSO, and Slack configuration.