Conversation
|
Caution Review failedPull request was closed or merged during review 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated GitHub Actions reusable workflow references to v2.17.0, added an e2e delete-extra-resources input with a listed resource, adjusted multiple timeouts, simplified the e2e test cleanup and expanded AutoEKSCluster + Observe manifests, removed a Python metadata generator, and replaced Makefile generation with a hops CLI call. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Reusable as reusable/e2e@v2.17.0
participant E2EJob as E2E Job
participant AWS as AWS APIs
GH->>Reusable: trigger e2e (inputs: delete-root-resource, delete-extra-resources, timeouts)
Reusable->>E2EJob: start tests and cleanup sequence
E2EJob->>AWS: create/test cluster, then delete extra resources and cluster
AWS-->>E2EJob: confirm deletion events
E2EJob-->>Reusable: report results
Reusable-->>GH: workflow completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Published Crossplane PackageThe following Crossplane package was published as part of this PR: Package: ghcr.io/hops-ops/stack-aws-observe:pr-1-efa70a78615e028641ac36b3882e3d33bf0e0f37 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/on-pr.yaml:
- Line 45: Change the mutable branch ref used in the reusable workflow
invocation (the "uses:
unbounded-tech/workflows-crossplane/.github/workflows/e2e.yaml@feat/delete-extra-resources"
line) to an immutable ref: replace the branch name
"@feat/delete-extra-resources" with either a commit SHA or a stable version tag
(for example "@v0.10.0" or a specific full commit SHA) so the referenced
e2e.yaml workflow is pinned and CI is reproducible.
- Around line 60-62: The workflow step timeout in .github/workflows/on-pr.yaml
is 33 minutes (timeout-minutes: 33) but tests/e2etest-observe/main.k declares
much larger timeouts (timeoutSeconds and cleanupTimeoutSeconds), so adjust the
test config to fit the workflow window: open tests/e2etest-observe/main.k and
reduce timeoutSeconds and cleanupTimeoutSeconds so the total test + cleanup is
safely under 33 minutes (e.g., set timeoutSeconds to ~1800 and
cleanupTimeoutSeconds to a small value like ~300), or alternatively increase the
workflow's timeout-minutes to at least 120 if you prefer keeping long test
timeouts; update either the workflow timeout-minutes or the
timeoutSeconds/cleanupTimeoutSeconds in main.k so they are consistent.
In `@tests/e2etest-observe/main.k`:
- Around line 225-226: The cleanup comment lists manifests "AutoEKSCluster" and
"Observe" but only "Observe" is defined; either remove "AutoEKSCluster" from the
comment or add the missing AutoEKSCluster manifest so they stay in sync—update
the header comment to accurately reflect the manifests (e.g., mention only
Observe and persistent hops-test network or include the AutoEKSCluster manifest
block) to avoid future cleanup confusion.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/on-pr.yaml.github/workflows/on-push-main.yaml.github/workflows/on-version-tagged.yamltests/e2etest-observe/main.k
.github/workflows/on-pr.yaml
Outdated
| timeout-minutes: 33 | ||
| cleanup-timeout-minutes: 20 | ||
| delete-extra-resources-timeout-minutes: 20 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 1: Check test-level timeout declarations ==="
if [ -f "tests/e2etest-observe/main.k" ]; then
echo "File exists. Searching for timeout declarations:"
rg -i 'timeout' tests/e2etest-observe/main.k -A 2 -B 2 || echo "No timeout patterns found"
else
echo "File tests/e2etest-observe/main.k not found"
fi
echo ""
echo "=== Step 2: Check workflow timeout values ==="
if [ -f ".github/workflows/on-pr.yaml" ]; then
echo "Workflow on-pr.yaml timeouts:"
rg -n 'timeout' .github/workflows/on-pr.yaml -B 2 -A 1 || echo "No timeout patterns found"
else
echo "File .github/workflows/on-pr.yaml not found"
fi
echo ""
echo "=== Step 3: List all test-related files ==="
find tests -name "*.k" -o -name "*.yaml" -o -name "*.yml" | head -20Repository: hops-ops/stack-aws-observe
Length of output: 844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full context of on-pr.yaml around lines 60-62 ==="
sed -n '50,75p' .github/workflows/on-pr.yaml
echo ""
echo "=== Check if there's a reusable workflow being called ==="
rg -n 'uses:|with:' .github/workflows/on-pr.yaml | head -20
echo ""
echo "=== Search for how test-observe job is configured ==="
rg -n 'test-observe|e2etest-observe' .github/workflows/on-pr.yaml -B 3 -A 10Repository: hops-ops/stack-aws-observe
Length of output: 1252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Get broader context to see which job has these timeouts ==="
sed -n '40,75p' .github/workflows/on-pr.yaml
echo ""
echo "=== Check job names in on-pr.yaml ==="
rg -n '^ [a-z]' .github/workflows/on-pr.yaml | head -20
echo ""
echo "=== Full e2e job section ==="
sed -n '44,75p' .github/workflows/on-pr.yamlRepository: hops-ops/stack-aws-observe
Length of output: 2107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Fetching reusable workflow to understand timeout input usage ==="
curl -sSf https://raw.githubusercontent.com/unbounded-tech/workflows-crossplane/feat/delete-extra-resources/.github/workflows/e2e.yaml 2>&1 | head -150Repository: hops-ops/stack-aws-observe
Length of output: 4609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Fetching full reusable workflow to see how timeout inputs are used ==="
curl -sSf https://raw.githubusercontent.com/unbounded-tech/workflows-crossplane/feat/delete-extra-resources/.github/workflows/e2e.yaml 2>&1 | grep -A 100 "jobs:" | head -200Repository: hops-ops/stack-aws-observe
Length of output: 3922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for where timeout-minutes input is used in e2e.yaml ==="
curl -sSf https://raw.githubusercontent.com/unbounded-tech/workflows-crossplane/feat/delete-extra-resources/.github/workflows/e2e.yaml 2>&1 | grep -n 'timeout-minutes\|inputs.timeout' -A 3 -B 3Repository: hops-ops/stack-aws-observe
Length of output: 3553
Reduce test timeout values in tests/e2etest-observe/main.k to align with workflow step timeout.
The GitHub Actions step timeout (33 minutes) kills the test execution after 33 minutes, but the test configuration declares 90 minutes for test execution (timeoutSeconds = 5400) and 30 minutes for cleanup (cleanupTimeoutSeconds = 1800). The workflow will terminate the step before the test completes, causing failures. Either increase the workflow step timeout to at least 120 minutes or reduce the test configuration timeouts to fit within 33 minutes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/on-pr.yaml around lines 60 - 62, The workflow step timeout
in .github/workflows/on-pr.yaml is 33 minutes (timeout-minutes: 33) but
tests/e2etest-observe/main.k declares much larger timeouts (timeoutSeconds and
cleanupTimeoutSeconds), so adjust the test config to fit the workflow window:
open tests/e2etest-observe/main.k and reduce timeoutSeconds and
cleanupTimeoutSeconds so the total test + cleanup is safely under 33 minutes
(e.g., set timeoutSeconds to ~1800 and cleanupTimeoutSeconds to a small value
like ~300), or alternatively increase the workflow's timeout-minutes to at least
120 if you prefer keeping long test timeouts; update either the workflow
timeout-minutes or the timeoutSeconds/cleanupTimeoutSeconds in main.k so they
are consistent.
| # manifests: All XRs that need cleanup (AutoEKSCluster, Observe) | ||
| # Note: Uses persistent hops-test network - no Network manifest needed |
There was a problem hiding this comment.
Cleanup comment is out of sync with the manifests list.
Lines 225-226 say manifests cover AutoEKSCluster and Observe, but Lines 228-237 only define Observe. This can mislead later cleanup changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2etest-observe/main.k` around lines 225 - 226, The cleanup comment
lists manifests "AutoEKSCluster" and "Observe" but only "Observe" is defined;
either remove "AutoEKSCluster" from the comment or add the missing
AutoEKSCluster manifest so they stay in sync—update the header comment to
accurately reflect the manifests (e.g., mention only Observe and persistent
hops-test network or include the AutoEKSCluster manifest block) to avoid future
cleanup confusion.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2etest-observe/main.k (1)
210-214: Inconsistent syntax: mixing:and=in tags dict.Lines 211-212 use
=for key-value assignment, but Line 213 uses:. While KCL allows both, mixing them within the same dict reduces readability.♻️ Proposed fix for consistency
tags = { "e2etest" = "true" "test-run" = _now - "repo": "https://github.com/hops-ops/stack-aws-observe" + "repo" = "https://github.com/hops-ops/stack-aws-observe" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2etest-observe/main.k` around lines 210 - 214, The tags dictionary currently mixes assignment syntaxes (uses "=" for "e2etest" and "test-run" but ":" for "repo"); update the tags dict so all entries use the same syntax (e.g., replace the ":" on the "repo" entry with "=") to keep the "tags" block consistent—look for the tags = {...} block and the keys "e2etest", "test-run", and "repo" to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2etest-observe/main.k`:
- Around line 210-214: The tags dictionary currently mixes assignment syntaxes
(uses "=" for "e2etest" and "test-run" but ":" for "repo"); update the tags dict
so all entries use the same syntax (e.g., replace the ":" on the "repo" entry
with "=") to keep the "tags" block consistent—look for the tags = {...} block
and the keys "e2etest", "test-run", and "repo" to make the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 8: The Makefile currently hardcodes HOPS_CLI to a developer-local
absolute path; change its default to a portable command name like HOPS_CLI ?=
hops-cli and modify the generate-configuration recipe to validate the command
exists before use (e.g., run a quick check using command -v/which and exit with
a clear error message if not found) so the target fails fast with a helpful
message rather than trying to run a non-portable path; update references to
HOPS_CLI in the generate-configuration target to rely on this variable and the
pre-check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 184dc717-9b38-44f0-a28f-8b5b65f952da
📒 Files selected for processing (2)
Makefilescripts/generate_configuration_metadata.py
💤 Files with no reviewable changes (1)
- scripts/generate_configuration_metadata.py
Makefile
Outdated
| COMPOSITION := $(XRD_DIR)/composition.yaml | ||
| DEFINITION := $(XRD_DIR)/definition.yaml | ||
| CONFIGURATION := $(XRD_DIR)/configuration.yaml | ||
| HOPS_CLI ?= /Users/patrickleet/dev/hops-ops/hops/cli/target/debug/hops-cli |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HOPS_CLI definition and usage in Makefile =="
cat -n Makefile | head -30
echo
echo "== Exact HOPS_CLI definition and generate-configuration target =="
rg -n 'HOPS_CLI|generate-configuration' Makefile -A 5 -B 1
echo
echo "== Check for HOPS_CLI in GitHub workflows =="
fd '\.(yml|yaml)$' .github/workflows 2>/dev/null | head -5 || true
if [ -d ".github/workflows" ]; then
rg -n 'HOPS_CLI|hops-cli' .github/workflows -C2 || echo "No matches found in workflows"
fi
echo
echo "== Check if there's documentation about HOPS_CLI =="
fd -e md -e txt . | xargs grep -l 'HOPS_CLI\|hops-cli' 2>/dev/null | head -5 || echo "No docs found mentioning HOPS_CLI"Repository: hops-ops/stack-aws-observe
Length of output: 3016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Examining workflow files for make invocations =="
fd '\.(yml|yaml)$' .github/workflows -x cat -n {} \; | head -200
echo
echo "== Check specifically for 'make' or 'validate' in workflows =="
rg -n 'make|validate' .github/workflows -C3 || echo "No matches"Repository: hops-ops/stack-aws-observe
Length of output: 7190
Use a portable HOPS_CLI default and fail fast with a clear error.
Line 8 hardcodes a developer-local absolute path (/Users/patrickleet/dev/hops-ops/hops/cli/target/debug/hops-cli), making the generate-configuration target (line 23) unusable on most machines without manual override. The path will fail on CI systems, other developers' machines, and non-macOS environments. Default to a command name and validate availability before running it.
Suggested fix
-HOPS_CLI ?= /Users/patrickleet/dev/hops-ops/hops/cli/target/debug/hops-cli
+HOPS_CLI ?= hops-cli
@@
generate-configuration:
`@set` -euo pipefail; \
+ if ! [ -x "$(HOPS_CLI)" ] && ! command -v "$(HOPS_CLI)" >/dev/null 2>&1; then \
+ echo "Error: hops CLI not found. Set HOPS_CLI=/path/to/hops-cli or install hops-cli in PATH."; \
+ exit 1; \
+ fi; \
"$(HOPS_CLI)" config generate --path . --api-path "$(XRD_DIR)"Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 8, The Makefile currently hardcodes HOPS_CLI to a
developer-local absolute path; change its default to a portable command name
like HOPS_CLI ?= hops-cli and modify the generate-configuration recipe to
validate the command exists before use (e.g., run a quick check using command
-v/which and exit with a clear error message if not found) so the target fails
fast with a helpful message rather than trying to run a non-portable path;
update references to HOPS_CLI in the generate-configuration target to rely on
this variable and the pre-check.
|
cancelled pipeline run cause only change was using the released workflow instead of the branch it was being made on, and I don't wanna wait. |
1 participant
Summary by CodeRabbit
Chores
Tests