ART-21677: Push .repo files to public upstream for RHCOS images - #3179
ART-21677: Push .repo files to public upstream for RHCOS images#3179locriandev wants to merge 4 commits into
Conversation
ART has been asked to push the generated .repo files (art-signed.repo and art-unsigned.repo) to the public upstream repository (https://github.com/openshift/os) during rebase operations. This is needed because the upstream team does not have access to openshift-priv where these files are currently stored. This change adds: - New method _push_repo_files_to_public_upstream() that pushes .repo files to public upstream for RHCOS images - Optimization to skip push if files haven't changed and branch exists - Branch naming convention: art-repos-{IMAGE_NAME}-{MAJOR}.{MINOR} - Only affects these RHCOS images: - rhcos-node-image - rhcos-node-image-rhel10 - rhcos-node-extensions - rhcos-node-extensions-rhel10 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
The previous implementation used the HTTPS URL from the 'web' field, which fails during git push with authentication errors. Converting to SSH format (git@github.com:org/repo.git) enables proper SSH key authentication for push operations. Changes: - Convert HTTPS URL to SSH format before cloning - Use SSH URL for both ls-remote check and clone operations - Format: https://github.com/org/repo -> git@github.com:org/repo.git Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
The git_clone helper automatically converts SSH URLs to HTTPS by design (to avoid pushing to public openshift repos). This causes authentication failures when trying to push. Solution: Clone with HTTPS (default behavior), then add a separate SSH remote specifically for push operations. This allows: - Clone via HTTPS (read-only, works with existing auth) - Push via SSH (write access, uses SSH keys) Changes: - Clone using web_url (HTTPS, gets converted by git_clone) - Add 'push_remote' with SSH URL after cloning - Push to 'push_remote' instead of 'origin' Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
The branch should contain only the .repo files without any parent commits or other files from the upstream repository. This keeps the branch clean and minimal. Changes: - Use --orphan flag when creating new branch - Run 'git rm -rf .' to clear all staged files from orphan branch - Only add the two .repo files to the orphan branch Result: Branch contains only art-signed.repo and art-unsigned.repo Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
WalkthroughThe rebaser now propagates RHCOS ChangesPublic RHCOS repository synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KonfluxRebaser
participant BuildRepository
participant OpenshiftOS
KonfluxRebaser->>BuildRepository: push rebase commit
KonfluxRebaser->>BuildRepository: check .oit/*.repo changes
KonfluxRebaser->>OpenshiftOS: check branch and clone upstream
KonfluxRebaser->>OpenshiftOS: copy art-signed.repo and art-unsigned.repo
KonfluxRebaser->>OpenshiftOS: commit and push branch
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
locriandev
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@locriandev: This pull request references ART-21677 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@locriandev: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![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
🧹 Nitpick comments (1)
doozer/tests/backend/test_rebaser.py (1)
2250-2307: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise and assert the synchronization paths directly.
The negative test has no “Git not called” assertions, while the positive test aborts at cloning and only checks
.called. It therefore does not verifyls-remote, orphan checkout, two-file staging, commit/push, or the existing-branch skip path. Add explicit awaited-call assertions and at least one successful end-to-end mocked synchronization.🤖 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 `@doozer/tests/backend/test_rebaser.py` around lines 2250 - 2307, Expand the tests around _push_repo_files_to_public_upstream to assert that non-RHCOS inputs make no Git calls, and that the RHCOS path awaits ls-remote, orphan checkout, stages both expected files, commits, and pushes. Replace the clone-failure-only positive test with successful mocked synchronization, and add coverage for an existing remote branch that skips synchronization; use explicit awaited-call assertions on the relevant git_helper symbols.
🤖 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 `@doozer/doozerlib/backend/rebaser.py`:
- Around line 1991-2008: Update the rebase flow around git_helper.git_clone so
the potentially long clone executes asynchronously without blocking the event
loop, using an async subprocess or exectools.to_thread; preferably limit the
operation to the required branch with a shallow fetch. Preserve the existing
HTTPS clone behavior, error logging, and exception propagation.
- Around line 235-237: Update _push_repo_files_to_public_upstream and its
related logic to compare the two generated .repo files directly against the
public branch rather than using HEAD~1..HEAD. Validate the diff, ls-remote, and
commit command results, treating only an explicit no-difference result as a
no-op; propagate all other Git failures so retries cannot silently skip
synchronization.
- Around line 2018-2029: Update the synchronization logic around the repo_files
loop to validate that both art-signed.repo and art-unsigned.repo source files
exist before copying or staging either file. If either is missing, fail the
synchronization immediately and atomically instead of continuing; only perform
shutil.copy2 and git add after all required sources pass validation.
---
Nitpick comments:
In `@doozer/tests/backend/test_rebaser.py`:
- Around line 2250-2307: Expand the tests around
_push_repo_files_to_public_upstream to assert that non-RHCOS inputs make no Git
calls, and that the RHCOS path awaits ls-remote, orphan checkout, stages both
expected files, commits, and pushes. Replace the clone-failure-only positive
test with successful mocked synchronization, and add coverage for an existing
remote branch that skips synchronization; use explicit awaited-call assertions
on the relevant git_helper symbols.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6f892ef-02c3-41bb-aa66-a90047fa583f
📒 Files selected for processing (2)
doozer/doozerlib/backend/rebaser.pydoozer/tests/backend/test_rebaser.py
| # Push .repo files to public upstream for RHCOS images | ||
| # This is called AFTER the push to ensure the commit exists and we can check if files changed | ||
| await self._push_repo_files_to_public_upstream(metadata, build_repo) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Base synchronization decisions on public-branch content, not the latest local commit.
The build repository is pushed first. If the public push then fails, a retry can create an empty commit; HEAD~1..HEAD reports no changes and the existing public branch is skipped indefinitely. Git failures are also conflated with expected states because the diff, ls-remote, and commit return codes are not validated.
Compare the two generated files against the public branch and treat only an explicit no-diff result as a no-op; propagate all other Git failures.
Also applies to: 1949-1978, 2031-2044
🤖 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 `@doozer/doozerlib/backend/rebaser.py` around lines 235 - 237, Update
_push_repo_files_to_public_upstream and its related logic to compare the two
generated .repo files directly against the public branch rather than using
HEAD~1..HEAD. Validate the diff, ls-remote, and commit command results, treating
only an explicit no-difference result as a no-op; propagate all other Git
failures so retries cannot silently skip synchronization.
| # Clone the PUBLIC upstream repository to a temporary directory | ||
| # Note: git_clone will convert SSH to HTTPS by default (by design) | ||
| with TemporaryDirectory() as temp_dir: | ||
| upstream_dir = Path(temp_dir) / "upstream" | ||
|
|
||
| try: | ||
| # Clone using HTTPS (git_clone converts SSH to HTTPS automatically) | ||
| git_helper.git_clone(web_url, str(upstream_dir)) | ||
| except Exception as e: | ||
| self._logger.error(f"Failed to clone public upstream repo {web_url}: {e}") | ||
| raise | ||
|
|
||
| # Create or checkout the branch | ||
| with exectools.Dir(str(upstream_dir)): | ||
| # Add SSH remote for pushing (we don't want to push to openshift/ via HTTPS) | ||
| self._logger.info(f"Adding SSH remote for push: {ssh_url}") | ||
| exectools.cmd_assert(["git", "remote", "add", "push_remote", ssh_url]) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move the synchronous clone off the async event loop.
git_clone performs a potentially long full clone of openshift/os synchronously, blocking other concurrent rebases. Use an async subprocess or exectools.to_thread, preferably with a branch-specific shallow fetch.
🤖 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 `@doozer/doozerlib/backend/rebaser.py` around lines 1991 - 2008, Update the
rebase flow around git_helper.git_clone so the potentially long clone executes
asynchronously without blocking the event loop, using an async subprocess or
exectools.to_thread; preferably limit the operation to the required branch with
a shallow fetch. Preserve the existing HTTPS clone behavior, error logging, and
exception propagation.
| # Copy .repo files from build repo to upstream repo root | ||
| repo_files = ["art-signed.repo", "art-unsigned.repo"] | ||
| for repo_file in repo_files: | ||
| src_path = build_repo.local_dir / ".oit" / repo_file | ||
| dst_path = upstream_dir / repo_file | ||
|
|
||
| if not src_path.exists(): | ||
| self._logger.warning(f"Source file not found: {src_path}") | ||
| continue | ||
|
|
||
| shutil.copy2(src_path, dst_path) | ||
| exectools.cmd_assert(["git", "add", repo_file]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require both .repo files before staging the update.
Continuing after a missing source file can create an incomplete orphan branch or retain a stale file on an existing branch. Validate both files up front and fail the synchronization atomically.
Proposed fix
repo_files = ["art-signed.repo", "art-unsigned.repo"]
+missing_files = [
+ repo_file
+ for repo_file in repo_files
+ if not (build_repo.local_dir / ".oit" / repo_file).is_file()
+]
+if missing_files:
+ raise FileNotFoundError(
+ f"Required public repo files are missing: {', '.join(missing_files)}"
+ )
+
for repo_file in repo_files:
src_path = build_repo.local_dir / ".oit" / repo_file
dst_path = upstream_dir / repo_file
-
- if not src_path.exists():
- self._logger.warning(f"Source file not found: {src_path}")
- continue
shutil.copy2(src_path, dst_path)
exectools.cmd_assert(["git", "add", repo_file])📝 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.
| # Copy .repo files from build repo to upstream repo root | |
| repo_files = ["art-signed.repo", "art-unsigned.repo"] | |
| for repo_file in repo_files: | |
| src_path = build_repo.local_dir / ".oit" / repo_file | |
| dst_path = upstream_dir / repo_file | |
| if not src_path.exists(): | |
| self._logger.warning(f"Source file not found: {src_path}") | |
| continue | |
| shutil.copy2(src_path, dst_path) | |
| exectools.cmd_assert(["git", "add", repo_file]) | |
| # Copy .repo files from build repo to upstream repo root | |
| repo_files = ["art-signed.repo", "art-unsigned.repo"] | |
| missing_files = [ | |
| repo_file | |
| for repo_file in repo_files | |
| if not (build_repo.local_dir / ".oit" / repo_file).is_file() | |
| ] | |
| if missing_files: | |
| raise FileNotFoundError( | |
| f"Required public repo files are missing: {', '.join(missing_files)}" | |
| ) | |
| for repo_file in repo_files: | |
| src_path = build_repo.local_dir / ".oit" / repo_file | |
| dst_path = upstream_dir / repo_file | |
| shutil.copy2(src_path, dst_path) | |
| exectools.cmd_assert(["git", "add", repo_file]) |
🤖 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 `@doozer/doozerlib/backend/rebaser.py` around lines 2018 - 2029, Update the
synchronization logic around the repo_files loop to validate that both
art-signed.repo and art-unsigned.repo source files exist before copying or
staging either file. If either is missing, fail the synchronization immediately
and atomically instead of continuing; only perform shutil.copy2 and git add
after all required sources pass validation.
2 participants
Summary
Add support for pushing generated .repo files (art-signed.repo and art-unsigned.repo) to the public upstream repository (https://github.com/openshift/os) during RHCOS image rebase operations.
Background
The upstream team does not have access to openshift-priv where .repo files are currently stored. They need these files in the public upstream repository to properly configure yum repositories during image builds.
Changes
Affected Images
All these images have
git@github.com:openshift-priv/os.gitas source URL andhttps://github.com/openshift/osas public web URL.Workflow
git ls-remoteTesting
https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-konflux/41119/ created https://github.com/openshift/os/tree/art-repos-rhcos-node-extensions-5.0 with the generated .repo files in it.