No issue #44
No issue #44
Conversation
Signed-off-by: noga-magen <nmagen@redhat.com>
WalkthroughBuild process for the Ansible collection was moved out of the repository tree into a temporary workspace. The script now defines VERSION and COLLECTION_ROOT, generates a transient galaxy.yml with overridden fields, performs a non-mutating rsync copy into a temp dir, verifies ansible-galaxy presence, and builds the collection into a temp OUTPUT_DIR. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as CI Runner
participant Script as ci/downstream.sh
participant Rsync as rsync (copy)
participant Tmp as Temp Dir (_tmp_dir)
participant Galaxy as ansible-galaxy
participant Artifacts as OUTPUT_DIR
Runner->>Script: invoke (VERSION, COLLECTION_ROOT)
Script->>Script: check ansible-galaxy in PATH
Script->>Tmp: mktemp & setup cleanup trap
Script->>Rsync: copy COLLECTION_ROOT -> Tmp (exclude .git, build, venv, etc.)
Script->>Tmp: write generated galaxy.yml (version/ns/name)
Script->>Galaxy: build collection from Tmp --output-path Artifacts
Galaxy-->>Artifacts: place built artifact
Script-->>Runner: exit (status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
ci/downstream.sh (2)
11-11: Sed-based YAML mutation is brittle and unescaped; consider yq or at least parameterize namespace/nameDirect sed replacements on YAML are fragile and will misbehave if VERSION contains special characters (e.g., “/” or “&”). Also, you’re hardcoding namespace/name, which makes forks harder.
Two options:
- Preferred: use yq to set keys safely (if available in the CI image):
- sed "s/^version: .*/version: $VERSION/; s/^namespace: .*/namespace: redhat/; s/^name: .*/name: edge_manager/" "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml" + yq -y \ + ".version = strenv(VERSION) | + .namespace = env(NAMESPACE) | + .name = env(NAME)" \ + "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml"
- Or, minimally, parameterize namespace/name for forks and keep sed:
+ NAMESPACE="${NAMESPACE:-redhat}" + NAME="${NAME:-edge_manager}" - sed "s/^version: .*/version: $VERSION/; s/^namespace: .*/namespace: redhat/; s/^name: .*/name: edge_manager/" "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml" + sed "s/^version: .*/version: $VERSION/; s/^namespace: .*/namespace: ${NAMESPACE}/; s/^name: .*/name: ${NAME}/" "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml"If you stick with sed, consider escaping replacements to handle “/” and “&”.
4-5: Make VERSION, NAMESPACE, NAME env-overridable for CI flexibilityAllowing env overrides makes the script usable across forks and release jobs without positional args.
Apply:
-VERSION="${1:-1.2.0}" +VERSION="${VERSION:-${1:-1.2.0}}" COLLECTION_ROOT="${2:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" +NAMESPACE="${NAMESPACE:-redhat}" +NAME="${NAME:-edge_manager}"You can then reference $NAMESPACE/$NAME in the metadata mutation step (see prior comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ci/downstream.sh(1 hunks)
Signed-off-by: noga-magen <nmagen@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ci/downstream.sh (4)
12-20: rsync is a hard dependency here — add fallback or preflightIf rsync isn’t available in the CI image, the script will fail. Either check for rsync and fall back to cp -a, or preflight and exit with a clear message.
Apply this diff to add a simple fallback:
-rsync -a --delete \ - --exclude '.git' \ - --exclude '.github' \ - --exclude 'build' \ - --exclude '.tox' \ - --exclude 'venv' \ - --exclude '.venv' \ - "${COLLECTION_ROOT}/" "${_tmp_dir}/" +if command -v rsync >/dev/null 2>&1; then + rsync -a --delete \ + --exclude '.git' \ + --exclude '.github' \ + --exclude 'build' \ + --exclude '.tox' \ + --exclude 'venv' \ + --exclude '.venv' \ + "${COLLECTION_ROOT}/" "${_tmp_dir}/" +else + # Fallback without excludes; acceptable for builds, but consider adding a .galaxyignore if needed. + cp -a "${COLLECTION_ROOT}/." "${_tmp_dir}/" +fi
22-23: Hard-coding name/namespace — confirm intent; consider only bumping versionGiven the PR’s goal (fixing a redundant part that led to a wrong artifact name), overwriting name and namespace here may reintroduce drift from galaxy.yml. If galaxy.yml already holds the correct values, limit the override to version.
Apply this diff if you intend to rely on galaxy.yml for name/namespace:
-sed "s/^version: .*/version: $VERSION/; s/^namespace: .*/namespace: redhat/; s/^name: .*/name: edge_manager/" "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml" +sed -E "s/^version:\s*.*/version: ${VERSION}/" "$COLLECTION_ROOT/galaxy.yml" > "$_tmp_dir/galaxy.yml"If you do need overrides, consider parameterizing them (e.g., NAME/NAMESPACE env vars) instead of hard-coding.
25-28: Minor: make builds idempotent and surface conflictsAdding --force prevents build failures on re-runs when the artifact already exists in OUTPUT_DIR. Also, consider printing the produced filename for CI logs.
Apply this diff:
-ansible-galaxy collection build "$_tmp_dir" --output-path "$OUTPUT_DIR" +ansible-galaxy collection build "$_tmp_dir" --output-path "$OUTPUT_DIR" --force
4-5: VERSION/COLLECTION_ROOT defaults look fine; consider single-source-of-truth for versionOptional: if $1 isn’t provided, you could default VERSION from galaxy.yml to avoid drift, and bail early if galaxy.yml is missing under COLLECTION_ROOT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ci/downstream.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-unit-downstream
- GitHub Check: test-sanity-downstream
- GitHub Check: integration-tests (stable-2.16, 3.12)
This comment was marked as resolved.
This comment was marked as resolved.
2 participants
I noticed when I try to upload the collection to automation-hub that the name of the file was flightctl-core.
Also I removed redundant part.
Summary by CodeRabbit