MGMT-19400 - create tag management flow to tag assisted installer components according to capbcoa versions #84
Conversation
openshift-ci
bot
added
the
size/XL
scripts/version_discovery.py
Outdated
| if tag_format == "clusterctl": | ||
| logger.info(f"Checking {repo} using clusterctl-compatible versioning") |
There was a problem hiding this comment.
What does clusterctl-compatible versioning mean? What's the difference with regular versioning, and why clusterctl in the name?
There was a problem hiding this comment.
yes that log is phrased horribly.
What I meant is that when searching for commits of assisted components we search for commit shas, but the capi and capm3
(which i called clusterctl because they are used by clusterctl in this line
shell: "clusterctl init --core cluster-api:{{ capi_version }} --bootstrap - --control-plane - --infrastructure metal3:{{ capm3_version }}"
components use regular versioning (like v1.2.3).
it means we just need to distinguish exactly what versions we are looking for, commits (in case of assisted) or releases (in case of capi/capm3)
There was a problem hiding this comment.
I think we can simplify and automatically detect if it's an hash or a tag (or maybe try to fetch hash, if error try tag?) this way it'd work for both (i.e. I want to try an already tagged assisted-components and I don't want to use the sha or I want to try a specific sha in an upstream component)
omer-vishlitzky
force-pushed
the
master
branch
2 times, most recently
from
fc2c22d to
ce45bc0
Compare
test/ansible/run_test.yaml
Outdated
| - name: prepare infrastructure-operator | ||
| delegate_to: localhost | ||
| shell: | | ||
| envsubst < "{{ playbook_dir }}/../e2e/manifests/infrastructure-operator/kustomization.yaml" > kustomization.yaml.new |
There was a problem hiding this comment.
nit: we migth want to save this as /e2e/manifests/infrastructure-operator/kustomization.yaml.tpl, so it's clear that it's not an usable kustomization file, and we would need only one step here.
Probably we could do the same with other files, but it'd be outside the scope of this PR.
There was a problem hiding this comment.
Also, we might want to do this in the target machine instead, so we can leave local copy clean
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
omer-vishlitzky
force-pushed
the
master
branch
5 times, most recently
from
9c49db8 to
c691f1b
Compare
scripts/tag_reconciler.py
Outdated
| return success | ||
|
|
||
| def reconcile_tags(self) -> bool: | ||
| versions_data = self.read_versions_file() |
There was a problem hiding this comment.
Can we document somwehere the expected format of this file? With a test would be ideal, if complicated a README would do
There was a problem hiding this comment.
Actually I think defining a type for each version and the version file could be clearer.
As generic remark I think we need in the version (and candidate version) file the git repo/git ref pairs. This could come both as a tag or as a commit ref, so it'd be the same for owned and not owned repos.
Then we could use another utility to transform this values in images if need be (to fetch the right sha)
scripts/test_runner.py
Outdated
| logger.info("Starting test runner") | ||
| data = read_release_candidates() | ||
|
|
||
| snapshot_index, snapshot = find_pending_snapshot(data) |
There was a problem hiding this comment.
I think we can do better than index here, maybe some name (date-based? ) that would make it easier to understand what's being processed.
Also please clarify what is the expected format (test or otherwise)
omer-vishlitzky
force-pushed
the
master
branch
2 times, most recently
from
b9e3ce6 to
7458047
Compare
openshift-ci
bot
added
size/XXL
omer-vishlitzky
force-pushed
the
master
branch
4 times, most recently
from
f585ffe to
1210670
Compare
openshift-merge-robot
added
the
needs-rebase
omer-vishlitzky
force-pushed
the
master
branch
3 times, most recently
from
f61f283 to
777bad9
Compare
omer-vishlitzky
force-pushed
the
master
branch
2 times, most recently
from
9626359 to
97f843d
Compare
openshift-merge-robot
removed
the
needs-rebase
.gitignore
Outdated
| @@ -25,3 +25,4 @@ go.work | |||
| *.swp | |||
| *.swo | |||
| *~ | |||
| scripts/__pycache__/* | |||
There was a problem hiding this comment.
Let's move all the scripts inside the hack/ folder, as it's a common pattern in golang projects it would be a less surprising place to find them
scripts/tag_reconciler.py
Outdated
| repo_name: str = component["repository"] | ||
| ref: str = component["ref"] | ||
|
|
||
| if not re.match(r"^openshift/", repo_name) or not ref: | ||
| logger.warning(f"Skipping repository {repo_name}") | ||
| continue | ||
|
|
||
| base_repo = self.extract_base_repo(repo_name) | ||
| if self.tag_exists(base_repo, version_name): | ||
| logger.info(f"Tag {version_name} already exists for {base_repo}") | ||
| continue | ||
| if not self.create_tag(base_repo, ref, version_name): | ||
| logger.error(f"Failed to create tag {version_name} for {base_repo}") | ||
| success = False |
There was a problem hiding this comment.
Can be extracted in another method for better readability
scripts/test_ansible_test_runner.py
Outdated
| - repository: https://github.com/metal3-io/cluster-api-provider-metal3 | ||
| ref: v1.9.3 | ||
| image_url: | ||
| - repository: assisted-service/assisted-service |
There was a problem hiding this comment.
Repository should be full nam,e of the repo, including github
scripts/test_ansible_test_runner.py
Outdated
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 | ||
| """ | ||
|
|
||
| with patch("builtins.open", mock_open(read_data=yaml_content)): |
There was a problem hiding this comment.
Wouldn't it better to actually have a test yaml file in some assets folder, so it would be more readable and we can actually read a file? This means we would need to parametrize the filename but I do not think that's an issue.
scripts/test_tag_reconciler.py
Outdated
| def test_read_versions_file_success(tag_reconciler): | ||
| yaml_content = """ | ||
| versions: | ||
| - name: v0.0.1 | ||
| components: | ||
| - repository: kubernetes-sigs/cluster-api | ||
| ref: v1.9.5 | ||
| image_url: | ||
| - repository: metal3-io/cluster-api-provider-metal3 | ||
| ref: v1.9.3 | ||
| image_url: | ||
| - repository: openshift/assisted-service/assisted-service | ||
| ref: 76d29d2a7f0899dcede9700fc88fcbad37b6ccca | ||
| image_url: quay.io/edge-infrastructure/assisted-service:latest-76d29d2a7f0899dcede9700fc88fcbad37b6ccca | ||
| - repository: openshift/assisted-image-service/assisted-image-service | ||
| ref: 2249c85d05600191b24e93dd92e733d49a1180ec | ||
| image_url: quay.io/edge-infrastructure/assisted-image-service:latest-2249c85d05600191b24e93dd92e733d49a1180ec | ||
| - repository: openshift/assisted-installer-agent/assisted-installer-agent | ||
| ref: cfe93a9779dea6ad2a628280b40071d23f3cb429 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-agent:latest-cfe93a9779dea6ad2a628280b40071d23f3cb429 | ||
| - repository: openshift/assisted-installer/assisted-installer-controller | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 | ||
| - repository: openshift/assisted-installer/assisted-installer | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 | ||
| """ |
There was a problem hiding this comment.
Same comments as candidate versions apply
scripts/version_discovery.py
Outdated
| if version_info: | ||
| tag_name, _ = version_info | ||
| repo_results.append({ | ||
| "repository": repo, | ||
| "ref": tag_name, | ||
| "image_url": None, | ||
| }) | ||
| elif images: | ||
| for image in images: | ||
| component_name = f"{repo}/{image.split('/')[-1]}" | ||
| logger.info(f"Checking {component_name} latest commit") | ||
|
|
||
| commit_info = None | ||
| try: | ||
| commit_info = self.find_latest_commit_with_image(repo, image) | ||
| except Exception as e: | ||
| logger.error(f"Error finding commit with image for {repo}/{image}: {e}") | ||
|
|
||
| if commit_info: | ||
| commit_sha, tag = commit_info | ||
| repo_results.append({ | ||
| "repository": repo, | ||
| "ref": commit_sha, | ||
| "image_url": f"{image}:{tag}", | ||
| }) | ||
| else: | ||
| logger.warning(f"No images found for {component_name}") | ||
| else: | ||
| logger.warning(f"No releases or tags found for {repo} and no images configured") | ||
|
|
||
| return repo_results |
There was a problem hiding this comment.
Can we simplify all those if-else and reduce nesting by returning early?
test/ansible/run_test.yaml
Outdated
| @@ -146,6 +146,15 @@ | |||
| - name: Debug tests output | |||
| debug: "msg={{ loadbalancerip.stdout }}" | |||
|
|
|||
| - name: prepare infrastructure-operator on target | |||
| shell: | | |||
| envsubst < "{{ src_dir }}/test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl" > "{{ remote_manifests_path }}/infrastructure-operator/kustomization.yaml" | |||
There was a problem hiding this comment.
Why output in {{ remote_manifests_path }}? BTW I think this might be unused, we can remove it
scripts/version_discovery.py
Outdated
| return repo_results | ||
|
|
||
| @staticmethod | ||
| def save_to_release_candidates( |
There was a problem hiding this comment.
I think we need to extract release candidate loading and saving.
I would recommend repository pattern, where we would have ReleaseCandidateRepository and a ReleaseCandidate object (structured or unstructured)
For example, something like:
rc_repo = ReleaseCandidateRepository("release_candidates.yaml)
rc_list = rc_repo.list()
rc_list.append(ReleaseCandidate({}))
rc_repo.save_list(rc_list)
The advantage is that we centralize the structure of this file: right now it's dispersed in multiple files and it makes it more difficult to understa.d
scripts/test_ansible_test_runner.py
Outdated
| yaml_content = """ | ||
| snapshots: | ||
| - metadata: | ||
| generated_at: '2025-03-10T10:32:04.642635' |
There was a problem hiding this comment.
we can leave generated_at, but I'd add a unique name/id (could be based on time, but since we have this field could be totally random now).
scripts/ansible_test_runner.py
Outdated
| logger.info("Starting test runner") | ||
| data: ReleaseCandidates = read_release_candidates() | ||
|
|
||
| timestamp, snapshot = find_pending_snapshot(data) |
There was a problem hiding this comment.
Again repo pattern would help here.
rc_list = rc_repo.find_release_candidates(status=pending)
# do my stuff with the pending candidates, modify the record(s)
rc_repo.save(id, rc_content)
WalkthroughThis pull request introduces multiple enhancements for release candidate testing and version management. The Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant SnapshotRepo
participant AnsibleTester
Main->>SnapshotRepo: find_pending_snapshot(data)
SnapshotRepo-->>Main: pending candidate found
Main->>Main: export_component_variables(snapshot)
Main->>AnsibleTester: run_ansible_tests()
AnsibleTester-->>Main: return test result (pass/fail)
Main->>SnapshotRepo: update_snapshot_status(snapshot_id, result)
sequenceDiagram
participant TagReconciler
participant GitHubClient
participant VersionsFile
TagReconciler->>VersionsFile: read_versions_file()
VersionsFile-->>TagReconciler: versions data retrieved
TagReconciler->>GitHubClient: tag_exists(repo, tag)?
alt Tag exists
GitHubClient-->>TagReconciler: tag found
else Tag missing
TagReconciler->>GitHubClient: create_tag(repo, commit_sha, tag)
GitHubClient-->>TagReconciler: tag created
end
TagReconciler->>TagReconciler: Process all version entries
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 4
🧹 Nitpick comments (21)
hack/test/assets/test_versions.yaml (1)
4-9: Consider standardizing repository referencesSome repositories use short references (e.g.,
kubernetes-sigs/cluster-api) while others use full URLs. For consistency and easier maintenance, consider standardizing all repository references to use the same format.hack/github_auth.py (1)
23-28: Consider simplifying validation logicThe validation logic could be simplified for better readability:
- if not all(bool(x) for x in [app_id, installation_id, private_key]): + missing_vars = [var for var, val in + {"GITHUB_APP_ID": app_id, + "GITHUB_APP_INSTALLATION_ID": installation_id, + "GITHUB_APP_PRIVATE_KEY": private_key}.items() + if not val] + if missing_vars: logger.error("Missing GitHub App credentials in environment") + raise ValueError(f"Missing GitHub App credentials: {', '.join(missing_vars)}") - raise ValueError(""" - Missing GitHub App credentials. - Ensure GITHUB_APP_ID, GITHUB_APP_INSTALLATION_ID, and GITHUB_APP_PRIVATE_KEY are set. - """)This approach provides more specific information about which variables are missing.
hack/test/assets/test_release_candidates.yaml (2)
2-5: Update test date to a non-future valueThe
generated_attimestamp is set to '2025-03-10', which is in the future. For test data, prefer using a date in the past or a placeholder like "2023-01-01".
7-15: Standardize repository URL formatAs in the test_versions.yaml file, repository references use inconsistent formats. Some include the full URL while others don't. Additionally, some URLs appear to have duplicated repository names:
https://github.com/openshift/assisted-serviceConsider standardizing all repository references to use the same format across both YAML files.
test/ansible/run_test.yaml (1)
13-14: Remove trailing whitespace in environment variable lookups.The variable definitions for
capi_versionandcapm3_versionhave trailing spaces after the closing quotes, which should be removed.- capi_version: "{{ lookup('ansible.builtin.env', 'CAPI_VERSION', default='v1.9.4') }}" - capm3_version: "{{ lookup('ansible.builtin.env', 'CAPM3_VERSION', default='v1.9.2') }}" + capi_version: "{{ lookup('ansible.builtin.env', 'CAPI_VERSION', default='v1.9.4') }}" + capm3_version: "{{ lookup('ansible.builtin.env', 'CAPM3_VERSION', default='v1.9.2') }}"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 14-14: trailing spaces
(trailing-spaces)
test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl (2)
12-12: Remove trailing whitespace after the variable placeholder.There's a trailing space after the variable placeholder that should be removed.
- digest: ${ASSISTED_SERVICE_IMAGE} + digest: ${ASSISTED_SERVICE_IMAGE}
32-32: Remove trailing whitespace after the variable placeholders.There are trailing spaces after the variable placeholders in multiple environment variable settings that should be removed for consistency.
- value: ${ASSISTED_IMAGE_SERVICE_IMAGE} + value: ${ASSISTED_IMAGE_SERVICE_IMAGE} - value: ${ASSISTED_INSTALLER_AGENT_IMAGE} + value: ${ASSISTED_INSTALLER_AGENT_IMAGE} - value: ${ASSISTED_INSTALLER_CONTROLLER_IMAGE} + value: ${ASSISTED_INSTALLER_CONTROLLER_IMAGE} - value: ${ASSISTED_INSTALLER_IMAGE} + value: ${ASSISTED_INSTALLER_IMAGE}Also applies to: 38-38, 41-41, 44-44
hack/test/test_tag_reconciler.py (1)
2-2: Remove unused importmock_open.The
mock_openimport is not used in this test file and should be removed.-from unittest.mock import MagicMock, patch, mock_open +from unittest.mock import MagicMock, patch🧰 Tools
🪛 Ruff (0.8.2)
2-2:
unittest.mock.mock_openimported but unusedRemove unused import:
unittest.mock.mock_open(F401)
hack/test/test_ansible_test_runner.py (3)
12-31: Increase coverage for edge cases intest_find_pending_snapshot.Your test correctly checks for a pending snapshot and a scenario where no pending snapshots exist. However, consider adding a scenario with missing or malformed
metadatakeys to verify robustness against unexpected input structures.
86-87: Combine nestedwithstatements to improve readability.You can merge these nested
withstatements into a singlewithblock:-with patch("os.path.exists", return_value=True): - with patch("builtins.open", m): +with patch("os.path.exists", return_value=True), patch("builtins.open", m): ...
109-110: Apply the same simplification for nestedwithstatements.-with patch("os.path.exists", return_value=False): - with patch("builtins.open", m): +with patch("os.path.exists", return_value=False), patch("builtins.open", m): ...hack/test/test_version_discovery.py (4)
1-2: Organize or remove unused imports if present.Check whether all imports (e.g.,
mock_open) are used in this file. If not, removing them emphasizes clarity and improves maintainability.
18-26: Consider adding timeout handling forrequests.head.While verifying image existence, you might want to test how
check_image_existsbehaves if a request times out. This ensures the code handles real-world network issues gracefully.
86-87: Simplify consecutivewithstatements.-with patch("os.path.exists", return_value=True): - with patch("builtins.open", m): +with patch("os.path.exists", return_value=True), patch("builtins.open", m): ...🧰 Tools
🪛 Ruff (0.8.2)
86-87: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
109-110: Apply the samewithstatement simplification here.-with patch("os.path.exists", return_value=False): - with patch("builtins.open", m): +with patch("os.path.exists", return_value=False), patch("builtins.open", m): ...🧰 Tools
🪛 Ruff (0.8.2)
109-110: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
hack/ansible_test_runner.py (2)
28-33: Handle potential missing keys infind_pending_snapshot.If
datadoes not contain"snapshots"or if elements lack"metadata", aKeyErrororTypeErrormay occur. Consider checks or defaults to prevent runtime errors.
77-99: Validated test execution flow inrun_ansible_tests.Capturing output and return codes ensures robust handling of both success and error paths. You might consider setting a reasonable timeout or capturing stdout/stderr for further logging.
hack/tag_reconciler.py (2)
9-13: Consider consolidating path setup logic.
It appears that both lines 9-13 and lines 23-26 undertake similar path resolution. You might refactor to avoid duplicatingscript_dir/project_rootreferences.- script_dir = os.path.dirname(os.path.abspath(__file__)) - project_root = os.path.dirname(script_dir) - if project_root not in sys.path: - sys.path.insert(0, project_root) SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) PROJECT_ROOT = os.path.dirname(SCRIPT_DIR) VERSIONS_FILE = os.path.join(PROJECT_ROOT, "versions.yaml")
52-60: Handle specific exceptions separately.
Currently, all exceptions are treated the same. Consider distinguishing 404 (tag not found) errors from other exceptions (e.g., network failure, rate limiting) to provide more accurate logs or retries.hack/version_discovery.py (1)
85-112: Consider handling authentication or rate-limiting.
While checking image existence via a HEAD request is valid, you may want to handle potential rate-limits or need for registry authentication, especially for private registries or frequent checks.hack/release_candidates_repository.py (1)
107-133: Ensure concurrency safety if needed.
Currently, a single YAML file stores and updates release candidate data. If multiple processes might update this file simultaneously, consider adding file locks or using a transactional approach to avoid race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)hack/ansible_test_runner.py(1 hunks)hack/github_auth.py(1 hunks)hack/release_candidates_repository.py(1 hunks)hack/shared_types.py(1 hunks)hack/tag_reconciler.py(1 hunks)hack/test/assets/test_release_candidates.yaml(1 hunks)hack/test/assets/test_versions.yaml(1 hunks)hack/test/test_ansible_test_runner.py(1 hunks)hack/test/test_tag_reconciler.py(1 hunks)hack/test/test_version_discovery.py(1 hunks)hack/version_discovery.py(1 hunks)test/ansible/run_test.yaml(2 hunks)test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
hack/test/test_tag_reconciler.py (1)
hack/tag_reconciler.py (6) (6)
TagReconciler(35:140)extract_base_repo(80:84)tag_exists(52:59)create_tag(61:78)reconcile_tag(103:127)read_versions_file(39:50)
hack/test/test_ansible_test_runner.py (2)
hack/release_candidates_repository.py (2) (2)
ReleaseCandidatesRepository(85:163)find_by_status(96:98)hack/ansible_test_runner.py (3) (3)
find_pending_snapshot(28:32)export_component_variables(35:74)run_ansible_tests(77:99)
hack/release_candidates_repository.py (1)
hack/shared_types.py (3) (3)
ReleaseCandidates(20:21)RepositoryInfo(5:8)SnapshotInfo(16:18)
🪛 Ruff (0.8.2)
hack/test/test_version_discovery.py
86-87: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
109-110: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
hack/test/test_tag_reconciler.py
2-2: unittest.mock.mock_open imported but unused
Remove unused import: unittest.mock.mock_open
(F401)
hack/test/test_ansible_test_runner.py
1-1: unittest.mock.mock_open imported but unused
Remove unused import: unittest.mock.mock_open
(F401)
🪛 YAMLlint (1.35.1)
test/ansible/run_test.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 14-14: trailing spaces
(trailing-spaces)
🔇 Additional comments (22)
.gitignore (1)
28-29: Appropriate additions to ignore Python bytecode cachesThese entries correctly exclude Python's bytecode cache directories from Git tracking, which is necessary when adding Python scripts to the repository.
hack/test/assets/test_versions.yaml (1)
6-9: Verify if missing image URLs are intentionalThe components for Kubernetes-related repositories don't have image URLs specified. Verify whether this is intentional or if these values should be provided.
hack/github_auth.py (1)
9-40: Well-implemented GitHub authentication functionThe function properly handles GitHub App authentication with good documentation, validation, and error handling.
test/ansible/run_test.yaml (1)
149-157: LGTM! Good implementation of dynamic component versioning.The new task successfully implements the infrastructure-operator template processing with environment variable lookups. This approach provides the necessary flexibility for the tag management flow, allowing different component versions to be specified dynamically.
test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl (1)
8-44: Great implementation of parameterized component versioning.The modifications to use variable placeholders instead of hardcoded digests align perfectly with the PR objective of implementing tag management for assisted installer components. This template-based approach provides the necessary flexibility to update component versions according to release requirements.
hack/test/test_tag_reconciler.py (1)
11-87: Well-structured tests for tag reconciliation functionality.The test suite provides good coverage of the
TagReconcilerclass functionality, testing individual methods as well as the complete tag reconciliation process. The tests are organized logically and use appropriate mocking to isolate the unit under test.The test for
reconcile_tageffectively verifies that tags are created when needed, which aligns with the PR objective of implementing tag management for assisted installer components.hack/shared_types.py (1)
1-33: Well-designed type definitions for tag management flow.The TypedDict classes provide a clear and structured representation of the data needed for the tag management flow. Using type hints enhances code quality by enabling static type checking and improving developer experience with better IDE support.
The hierarchy from individual repository information to snapshots and version entries logically organizes the data needed to implement tag management for assisted installer components based on capbcoa versions.
hack/test/test_ansible_test_runner.py (3)
1-1: Clarify the static analysis false positive.Ruff flags
mock_openas unused, but it's actually used on lines 84, 107, etc. This is a false positive and can be safely ignored.🧰 Tools
🪛 Ruff (0.8.2)
1-1:
unittest.mock.mock_openimported but unusedRemove unused import:
unittest.mock.mock_open(F401)
32-53: Good test coverage forexport_component_variables.The test comprehensively validates environment variable assignments. This helps ensure each mapped component is correctly set. No immediate changes needed here.
54-82: Effective verification of Ansible test runner.The test verifies both success and failure return codes with appropriate asserts. This is well-structured for capturing typical execution pathways.
hack/test/test_version_discovery.py (1)
10-17: Fixture implementation looks solid.Initializing the
ComponentScannerwith a mocked GitHub client is a neat approach that isolates the test environment effectively.hack/ansible_test_runner.py (3)
35-75: Thorough environment variable export logic.This function correctly handles mapping for multiple repositories and logs missing mappings as warnings. Overall, it is well-designed for maintainability.
102-119: Successful or failed snapshot updates are well-handled.The method logs results, updates statuses, and handles errors gracefully. This aligns with best practices for external data modifications.
121-149: Nice top-level orchestration inmain.Overall control flow is concise, logging is comprehensive, and error handling is robust. Good job.
hack/tag_reconciler.py (4)
39-51: Validate exit-on-missing file behavior.
The method immediately exits ifVERSIONS_FILEis not present. This may be desired, but it could disrupt other flows. Verify if halting the entire script is the intended behavior.Do you want to confirm that other callers or modules do not expect a fallback behavior when the file is missing?
86-102: Looks good!
Tag existence checks and creation logic appear correct and straightforward.
129-141: Overall flow is coherent.
Your approach to iterating over each version entry and applying the tag reconciliation is organized and clear.
103-128:✅ Verification successful
Confirm Python 3.9+ usage.
Theremoveprefixmethod is only available in Python 3.9 or newer. Ensure the runtime environment meets this requirement or use an alternative substring removal approach for broader compatibility.
🏁 Script executed:
#!/bin/bash # Verify Python version is >= 3.9 python --versionLength of output: 32
Python 3.9+ Environment Confirmed
The runtime environment is using Python 3.11.2, which fully supports theremoveprefixmethod. No changes are required.hack/version_discovery.py (2)
57-69: Efficient commit fetch.
Fetching the most recent commits and storing their SHAs is straightforward; no issues found here.
130-150: Revisit commit limit.
Only checking the 20 most recent commits might overlook valid images if they occurred earlier than 20 commits ago. Confirm that this limit is acceptable for your release flow.hack/release_candidates_repository.py (2)
32-56: Constructor logic appears correct.
Initialization, metadata setup, and dynamic UUID generation are well-handled.
73-83: Robust component equality check.
Using sets of tuples for component comparison is a reliable pattern.
hack/test/assets/test_versions.yaml
Outdated
| - repository: https://github.com/openshift/assisted-service/assisted-service | ||
| ref: 76d29d2a7f0899dcede9700fc88fcbad37b6ccca | ||
| image_url: quay.io/edge-infrastructure/assisted-service:latest-76d29d2a7f0899dcede9700fc88fcbad37b6ccca | ||
| - repository: https://github.com/openshift/assisted-image-service/assisted-image-service | ||
| ref: 2249c85d05600191b24e93dd92e733d49a1180ec | ||
| image_url: quay.io/edge-infrastructure/assisted-image-service:latest-2249c85d05600191b24e93dd92e733d49a1180ec | ||
| - repository: https://github.com/openshift/assisted-installer-agent/assisted-installer-agent | ||
| ref: cfe93a9779dea6ad2a628280b40071d23f3cb429 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-agent:latest-cfe93a9779dea6ad2a628280b40071d23f3cb429 | ||
| - repository: https://github.com/openshift/assisted-installer/assisted-installer-controller | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 | ||
| - repository: https://github.com/openshift/assisted-installer/assisted-installer | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 |
There was a problem hiding this comment.
Fix duplicated repository names in URLs
Several repository URLs appear to have the repository name duplicated at the end:
https://github.com/openshift/assisted-service/assisted-service
This format looks incorrect and might cause issues when working with these URLs. Consider removing the duplicated repository name:
- repository: https://github.com/openshift/assisted-service/assisted-service
+ repository: https://github.com/openshift/assisted-service📝 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.
| - repository: https://github.com/openshift/assisted-service/assisted-service | |
| ref: 76d29d2a7f0899dcede9700fc88fcbad37b6ccca | |
| image_url: quay.io/edge-infrastructure/assisted-service:latest-76d29d2a7f0899dcede9700fc88fcbad37b6ccca | |
| - repository: https://github.com/openshift/assisted-image-service/assisted-image-service | |
| ref: 2249c85d05600191b24e93dd92e733d49a1180ec | |
| image_url: quay.io/edge-infrastructure/assisted-image-service:latest-2249c85d05600191b24e93dd92e733d49a1180ec | |
| - repository: https://github.com/openshift/assisted-installer-agent/assisted-installer-agent | |
| ref: cfe93a9779dea6ad2a628280b40071d23f3cb429 | |
| image_url: quay.io/edge-infrastructure/assisted-installer-agent:latest-cfe93a9779dea6ad2a628280b40071d23f3cb429 | |
| - repository: https://github.com/openshift/assisted-installer/assisted-installer-controller | |
| ref: c389a38405383961d26191799161c86127451635 | |
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 | |
| - repository: https://github.com/openshift/assisted-installer/assisted-installer | |
| ref: c389a38405383961d26191799161c86127451635 | |
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 | |
| - repository: https://github.com/openshift/assisted-service | |
| ref: 76d29d2a7f0899dcede9700fc88fcbad37b6ccca | |
| image_url: quay.io/edge-infrastructure/assisted-service:latest-76d29d2a7f0899dcede9700fc88fcbad37b6ccca | |
| - repository: https://github.com/openshift/assisted-image-service/assisted-image-service | |
| ref: 2249c85d05600191b24e93dd92e733d49a1180ec | |
| image_url: quay.io/edge-infrastructure/assisted-image-service:latest-2249c85d05600191b24e93dd92e733d49a1180ec | |
| - repository: https://github.com/openshift/assisted-installer-agent/assisted-installer-agent | |
| ref: cfe93a9779dea6ad2a628280b40071d23f3cb429 | |
| image_url: quay.io/edge-infrastructure/assisted-installer-agent:latest-cfe93a9779dea6ad2a628280b40071d23f3cb429 | |
| - repository: https://github.com/openshift/assisted-installer/assisted-installer-controller | |
| ref: c389a38405383961d26191799161c86127451635 | |
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 | |
| - repository: https://github.com/openshift/assisted-installer/assisted-installer | |
| ref: c389a38405383961d26191799161c86127451635 | |
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 |
| - repository: https://github.com/openshift/assisted-installer | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 | ||
| - repository: https://github.com/openshift/assisted-installer | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer:latest-c389a38405383961d26191799161c86127451635 |
There was a problem hiding this comment.
Remove duplicate repository entries
There are duplicate entries for the "openshift/assisted-installer" repository (lines 22-27). These appear to be for different images (controller vs. regular), but using the same repository and ref. Consider either:
- Using distinct repository paths to reflect these are separate components, or
- Consolidating them into a single entry with multiple image references
This will prevent confusion and potential issues when processing this data.
There was a problem hiding this comment.
This guy has a point, should we add name property for components? This will solve this issue, possibly the non-writable components one too
There was a problem hiding this comment.
@rccrdpccl I agree that adding a name property to components is a good solution. This would allow you to distinguish between different components that share the same repository and ref, like the assisted-installer and assisted-installer-controller.
This change would require:
- Updating the
RepositoryInfotype inhack/shared_types.pyto include the new field - Modifying any code that processes these components to handle the name property
- Updating existing YAML files to include names for each component
The name field would provide clearer identification and could indeed help with both the duplicate repository issue and the non-writable components problem you mentioned.
✏️ Learnings added
Learnt from: rccrdpccl
PR: openshift-assisted/cluster-api-agent#84
File: hack/test/assets/test_release_candidates.yaml:22-27
Timestamp: 2025-03-19T08:01:00.682Z
Learning: Components in the release candidates YAML should have a `name` property to distinguish between different components that share the same repository and ref, such as assisted-installer and assisted-installer-controller.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@omer-vishlitzky Could you please address this? This will impact the rest of the codebase too
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
hack/shared_types.py
Outdated
| name: str | ||
| components: list[RepositoryInfo] | ||
|
|
||
| class VersionsFile(TypedDict): |
There was a problem hiding this comment.
What's the difference between VersionFile and VersionEntry? The way I see it is that the file is just where we save a list of released versions. I think we should not have the word file in the name.
I think we might rethink how we represent those. What's the relation between the handled objects? The way I see it is:
- we discover a new "version candidate" or "snapshot"
- we validate such snapshot, if valid can be promoted to versions
- once added to versions, it will be reconciled
Can we find a way to represent this?
There was a problem hiding this comment.
I understand your point.
WDYT of this?
The point I'm trying to convey is there is a snapshot which has a status, and a version is a snapshot that had been promoted.
I'm open to hear if you have any other suggestion here
class RepositoryInfo(TypedDict):
repository: str
ref: str
image_url: str | None
class SnapshotMetadata(TypedDict):
id: str
generated_at: str
status: str # "pending", "failed", "successful"
tested_at: str
class Snapshot(TypedDict):
metadata: SnapshotMetadata
components: list[RepositoryInfo]
class SnapshotCollection(TypedDict):
snapshots: list[Snapshot]
# a version is a promoted successful snapshot
class Version(TypedDict):
name: str
components: list[RepositoryInfo]
class VersionsCollection(TypedDict):
versions: list[Version]
class RepositoryConfig(TypedDict, total=False):
images: list[str] | None
version_prefix: str
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/ansible/run_test.yaml (1)
149-157: Use the Ansible template module rather thanshellfor better readability and error handling.You are using
shellwithenvsubstto substitute environment variables in thekustomization.yaml.tpl. This is functional but can hide potential errors. Consider using the Ansibletemplateorassemblemodules with Jinja2 templating to make it more robust and explicit.- - name: prepare infrastructure-operator on target - shell: | - envsubst < "{{ src_dir }}/test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl" > "{{ src_dir }}/test/e2e/manifests/infrastructure-operator/kustomization.yaml" - environment: - ASSISTED_SERVICE_IMAGE: "{{ lookup('ansible.builtin.env', 'ASSISTED_SERVICE_IMAGE', default='quay.io/edge-infrastructure/assisted-service@sha256:a78af04976f68f4e33b3a53001c959c15b7c350cb2b8836004066a40fd1e261d') }}" + - name: prepare infrastructure-operator on target (template-based) + template: + src: "kustomization.yaml.tpl" + dest: "{{ src_dir }}/test/e2e/manifests/infrastructure-operator/kustomization.yaml" + vars: + ASSISTED_SERVICE_IMAGE: "{{ lookup('ansible.builtin.env', 'ASSISTED_SERVICE_IMAGE', default='quay.io/edge-infrastructure/assisted-service@sha256:a78af04976f68f4e33b3a53001c959c15b7c350cb2b8836004066a40fd1e261d') }}"hack/test/test_ansible_test_runner.py (1)
1-1: Remove unused import.
mock_openis imported but never used, which may cause linting issues and confusion for readers.-from unittest.mock import patch, mock_open +from unittest.mock import patch🧰 Tools
🪛 Ruff (0.8.2)
1-1:
unittest.mock.mock_openimported but unusedRemove unused import:
unittest.mock.mock_open(F401)
hack/ansible_test_runner.py (1)
102-120: Add test coverage for snapshot status updates.It appears
update_snapshot_statusis not tested in the accompanying test suite. Including a unit test would ensure that status transitions (e.g., from "pending" to "successful" or "failed") are validated.Would you like me to generate a sample test function that covers this method?
hack/version_discovery.py (1)
151-167: Handle rate-limiting for concurrent GitHub requests.Using a
ThreadPoolExecutorwith up to 10 workers is efficient, but frequent or large repository scans can run into GitHub’s rate limit. Consider adding logic for retries or backoff upon encountering rate-limit responses (such as status code 403) to avoid partial data retrieval.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)hack/ansible_test_runner.py(1 hunks)hack/github_auth.py(1 hunks)hack/release_candidates_repository.py(1 hunks)hack/shared_types.py(1 hunks)hack/tag_reconciler.py(1 hunks)hack/test/assets/test_release_candidates.yaml(1 hunks)hack/test/assets/test_versions.yaml(1 hunks)hack/test/test_ansible_test_runner.py(1 hunks)hack/test/test_tag_reconciler.py(1 hunks)hack/test/test_version_discovery.py(1 hunks)hack/version_discovery.py(1 hunks)test/ansible/run_test.yaml(2 hunks)test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- .gitignore
- hack/test/assets/test_versions.yaml
- hack/github_auth.py
- hack/test/test_tag_reconciler.py
- hack/test/assets/test_release_candidates.yaml
- test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl
- hack/test/test_version_discovery.py
- hack/shared_types.py
- hack/tag_reconciler.py
- hack/release_candidates_repository.py
🧰 Additional context used
🧬 Code Definitions (2)
hack/version_discovery.py (3)
hack/github_auth.py (1) (1)
get_github_client(9-40)hack/shared_types.py (2) (2)
RepositoryConfig(30-32)RepositoryInfo(5-8)hack/release_candidates_repository.py (3) (3)
ReleaseCandidate(32-82)ReleaseCandidatesRepository(85-163)save(107-132)
hack/test/test_ansible_test_runner.py (2)
hack/release_candidates_repository.py (2) (2)
ReleaseCandidatesRepository(85-163)find_by_status(96-98)hack/ansible_test_runner.py (3) (3)
find_pending_snapshot(28-32)export_component_variables(35-74)run_ansible_tests(77-99)
🪛 Ruff (0.8.2)
hack/test/test_ansible_test_runner.py
1-1: unittest.mock.mock_open imported but unused
Remove unused import: unittest.mock.mock_open
(F401)
🔇 Additional comments (2)
test/ansible/run_test.yaml (1)
13-14: Confirm environment variable usage.Switching from hardcoded versions to environment-based configuration is a good approach for flexibility. However, please ensure that any relevant scripts or CI jobs set these environment variables appropriately (e.g., CAPI_VERSION, CAPM3_VERSION). Otherwise, it will default, which might not be desired in every scenario.
Would you like me to generate a shell script to search the surrounding CI/CD files for references to these environment variables?
hack/ansible_test_runner.py (1)
28-33: Return structure looks consistent.Returning
(snapshot_id, snapshot)or(None, None)fromfind_pending_snapshotis clearly defined and readable. This looks good, as it removes ambiguity about whether both or neither value might be set.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
hack/tag_reconciler.py (6)
39-50: Improve exception handling in read_versions_file methodWhen re-raising exceptions within an except clause, it's best practice to use
raise ... from esyntax to preserve the original exception context. This helps with debugging by maintaining the full exception chain.except Exception as e: - raise Exception(f"Error reading {VERSIONS_FILE}: {e}") + raise Exception(f"Error reading {VERSIONS_FILE}: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
49-49: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
79-84: Add validation for repository name formatThe
extract_base_repomethod assumes a specific repository name format. Consider adding more robust validation to handle edge cases like malformed repository names.def extract_base_repo(self, full_repo_name: str) -> str: + if not full_repo_name or '/' not in full_repo_name: + logger.warning(f"Invalid repository name format: {full_repo_name}") + return full_repo_name + parts = full_repo_name.split("/") if len(parts) > 2: return f"{parts[0]}/{parts[1]}" return full_repo_name
102-127: Consider making repository filtering configurableThe code currently only processes repositories that start with "openshift/". Consider making this filter configurable rather than hard-coded to improve flexibility and reusability.
def __init__(self): self.github_client = get_github_client() + # Default to openshift repositories, but allow configuration + self.repo_prefix = os.environ.get("REPO_PREFIX", "openshift/") # ... other methods ... def reconcile_tag(self, version: Version) -> bool: # ... existing code ... for component in components: repo_name: str = component["repository"].removeprefix("https://github.com/") ref: str = component["ref"] - if not re.match(r"^openshift/", repo_name) or not ref: + if not re.match(f"^{self.repo_prefix}", repo_name) or not ref: logger.warning(f"Skipping repository {repo_name}") continue # ... rest of the code ...
124-126: Consider detailed error reporting when reconciliation failsThe current implementation uses
success &= ...to track failures but doesn't provide detailed information about which specific tags failed to be reconciled. Consider collecting and reporting failures for better troubleshooting.def reconcile_tag(self, version: Version) -> bool: version_name: str = version["name"] if not version_name: logger.error("Version entry missing 'name' field") return False logger.info(f"Processing version {version_name}") success = True + failed_components = [] components = version["components"] if not components: logger.warning(f"No repositories found for version {version_name}") return True for component in components: repo_name: str = component["repository"].removeprefix("https://github.com/") ref: str = component["ref"] if not re.match(r"^openshift/", repo_name) or not ref: logger.warning(f"Skipping repository {repo_name}") continue - success &= self.ensure_tag_exists(ref, repo_name, version_name) + if not self.ensure_tag_exists(ref, repo_name, version_name): + success = False + failed_components.append(repo_name) + if not success: + logger.error(f"Failed to tag the following components for version {version_name}: {', '.join(failed_components)}") return success
35-159: Add docstrings to class and methodsThe code is well-structured, but adding docstrings to the class and each method would improve maintainability and make it easier for other developers to understand the purpose and usage of each component.
Here's an example for the class and one method:
class TagReconciler: + """ + Reconciles tags across GitHub repositories based on version information. + + This class reads version information from a YAML file, checks if tags exist in + repositories, creates tags if necessary, and reconciles tags across all repositories. + """ def __init__(self): self.github_client = get_github_client() def read_versions_file(self) -> VersionsCollection: + """ + Reads version information from the VERSIONS_FILE. + + Returns: + VersionsCollection: The parsed version information from the YAML file. + + Raises: + Exception: If the file doesn't exist or there's an error reading it. + """ if not os.path.exists(VERSIONS_FILE): logger.error(f"{VERSIONS_FILE} does not exist") sys.exit(1)Consider adding similar docstrings to all methods.
🧰 Tools
🪛 Ruff (0.8.2)
49-49: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
128-140: Consider adding retry mechanism for failed tagsFor production reliability, consider implementing a retry mechanism for failed tag reconciliations. Network issues or temporary GitHub API limitations could cause temporary failures.
def reconcile_tags(self) -> bool: versions_data: VersionsCollection = self.read_versions_file() versions: list[Version] = versions_data["versions"] logger.info(f"Processing {len(versions)} versions") success = True + failed_versions = [] for version_entry in versions: if not self.reconcile_tag(version_entry): success = False + failed_versions.append(version_entry["name"]) + # Retry failed versions once + if failed_versions: + logger.info(f"Retrying {len(failed_versions)} failed versions") + for version_name in failed_versions.copy(): + version_entry = next((v for v in versions if v["name"] == version_name), None) + if version_entry and self.reconcile_tag(version_entry): + failed_versions.remove(version_name) + + if failed_versions: + logger.error(f"The following versions failed after retry: {', '.join(failed_versions)}") return successhack/shared_types.py (1)
13-13: Consider using Literal type for status fieldWhile the comment indicates the possible values ("pending", "failed", "successful"), using a Literal type would provide better type safety and code completion.
- status: str # "pending", "failed", "successful" + from typing import Literal + status: Literal["pending", "failed", "successful"]hack/ansible_test_runner.py (1)
77-96: Consider parameterizing Ansible test path for better testabilityThe hardcoded paths to the Ansible playbook and inventory make this function difficult to test. Consider making these configurable or passing them as parameters.
-def run_ansible_tests() -> bool: +def run_ansible_tests(playbook_path: str = "test/ansible/run_test.yaml", inventory_path: str = "test/ansible/inventory.yaml") -> bool: try: logger.info("Running Ansible tests...") ansible_cmd = [ "ansible-playbook", - "test/ansible/run_test.yaml", + playbook_path, "-i", - "test/ansible/inventory.yaml", + inventory_path, ]hack/release_candidates_repository.py (1)
73-82: Consider documenting that component order is ignored in equality comparisonThe
equalsmethod converts components to sets, which means the order of components will be ignored during comparison. This might be intentional but should be documented.def equals(self, other: "ReleaseCandidate") -> bool: + # Compare components as sets, ignoring their order set1 = { (d.get("repository", ""), d.get("ref", ""), d.get("image_url", "")) for d in self.components } set2 = { (d.get("repository", ""), d.get("ref", ""), d.get("image_url", "")) for d in other.components } return set1 == set2hack/test/test_version_discovery.py (4)
18-26: Expand coverage to additional status codes.Currently, the test checks for 200 and 404 status codes. Consider adding checks for other edge cases (like 500 or 401) to ensure robust handling of unexpected Quay responses.
27-40: Add negative test for missing or all-prerelease tags.While this test covers ignoring prerelease tags, consider adding a scenario where no valid releases are found or all are prerelease, verifying that the method returns
None.
52-63: Consider adding failing branch coverage.It might be beneficial to add a test case verifying behavior when
find_latest_commit_with_imagereturnsNone, confirming the function gracefully handles missing images.
64-96: Ensure malformed YAML scenario is tested.Testing successful appends to an existing YAML file looks good, but consider a follow-up test for handling corrupted or malformed YAML, ensuring robust error handling.
hack/version_discovery.py (5)
26-50: Consider prefixing dictionary keys with owners.
REPOSITORIESuses a dictionary keyed by GitHub repository path strings. If you plan to expand supporting more organizations or custom naming, consider a more descriptive key structure. Otherwise, this is clear for now.
70-84: Release fetching logic is concise.The method properly limits fetched releases. You might want to log the total number of releases returned before slicing for added debugging clarity.
85-112: Extend registry checks if more image registries are needed.Currently, this method exclusively supports Quay. If multi-registry scanning arises in the future, factor out the registry logic for better extensibility.
130-150: Sufficient logic for commit-based image checks.The approach of scanning the latest commits and forming a tag is straightforward. If your image pipeline uses additional naming conventions, consider parameterizing the constructed tag string.
151-168: ThreadPoolExecutor usage is appropriate but watch GitHub rate limits.With a max of 10 workers against multiple repos, confirm your rate limit usage. If usage grows, consider adding rate-limit handling or caching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)hack/ansible_test_runner.py(1 hunks)hack/github_auth.py(1 hunks)hack/release_candidates_repository.py(1 hunks)hack/shared_types.py(1 hunks)hack/tag_reconciler.py(1 hunks)hack/test/assets/test_release_candidates.yaml(1 hunks)hack/test/assets/test_versions.yaml(1 hunks)hack/test/test_ansible_test_runner.py(1 hunks)hack/test/test_tag_reconciler.py(1 hunks)hack/test/test_version_discovery.py(1 hunks)hack/version_discovery.py(1 hunks)test/ansible/run_test.yaml(2 hunks)test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- hack/test/assets/test_versions.yaml
- hack/github_auth.py
- hack/test/test_tag_reconciler.py
- test/e2e/manifests/infrastructure-operator/kustomization.yaml.tpl
- test/ansible/run_test.yaml
- hack/test/test_ansible_test_runner.py
- hack/test/assets/test_release_candidates.yaml
🧰 Additional context used
🧬 Code Definitions (4)
hack/ansible_test_runner.py (2)
hack/release_candidates_repository.py (4) (4)
ReleaseCandidatesRepository(85-163)update_status(134-143)find_by_status(96-98)to_dict(57-58)hack/shared_types.py (2) (2)
SnapshotCollection(20-21)Snapshot(16-18)
hack/version_discovery.py (3)
hack/github_auth.py (1) (1)
get_github_client(9-40)hack/shared_types.py (2) (2)
RepositoryConfig(31-33)RepositoryInfo(5-8)hack/release_candidates_repository.py (3) (3)
ReleaseCandidate(32-82)ReleaseCandidatesRepository(85-163)save(107-132)
hack/release_candidates_repository.py (1)
hack/shared_types.py (3) (3)
SnapshotCollection(20-21)RepositoryInfo(5-8)Snapshot(16-18)
hack/tag_reconciler.py (2)
hack/github_auth.py (1) (1)
get_github_client(9-40)hack/shared_types.py (2) (2)
Version(24-26)VersionsCollection(28-29)
🪛 Ruff (0.8.2)
hack/tag_reconciler.py
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (26)
hack/tag_reconciler.py (4)
1-33: LGTM! Great setup with proper imports and configurationThe script has a solid foundation with the right imports, path setup, and YAML configuration. The logging configuration is properly set, which will help with troubleshooting during execution.
51-78: Robust implementation of GitHub tag managementThe tag existence checking and creation methods are well-implemented with proper error handling and logging. Good job including informative log messages that will help with diagnosing issues during execution.
85-101: Well-structured tag existence logicThe
ensure_tag_existsmethod logically separates the concern of checking for tag existence and creating tags when needed. The error handling is appropriate, with clear log messages indicating the state of operations.
142-156: Main function design looks goodThe main function properly initializes the reconciler, handles the reconciliation process, and provides appropriate error handling. The exit code handling will allow integration with CI/CD pipelines to detect failures.
hack/shared_types.py (4)
5-8: LGTM! Clear type definition for repository information.The
RepositoryInfoTypedDict is well-structured with appropriate fields for tracking repository metadata. The optionalimage_urlfield (withNoneas a possible value) allows for flexible repository representation.
16-18: LGTM! Well-structured Snapshot type.This type definition cleanly combines metadata and components, providing a clear structure for snapshots.
20-21: LGTM! Collection types clearly express the data structure.Both
SnapshotCollectionandVersionsCollectionuse consistent naming patterns and clearly express their purpose as collections of their respective types.Also applies to: 28-29
31-33: LGTM! Good use of total=False for optional fields.The
RepositoryConfigTypedDict correctly usestotal=Falseto indicate that both fields are optional. The| Noneon theimagesfield also allows for explicitly setting it toNone.hack/ansible_test_runner.py (4)
28-32: LGTM! Correct handling of snapshot search logic.This function correctly searches for pending snapshots and handles the case where a snapshot might be missing the metadata field.
35-47: Component-to-environment variable mapping looks good.The mapping between repository names and environment variables is well-defined and covers the necessary components for the assisted installer ecosystem.
102-119: LGTM! Proper error handling and status update logic.The function correctly updates the snapshot status and handles potential errors when interacting with the repository.
121-145: LGTM! Well-structured main function with proper error handling.The main function clearly orchestrates the process of finding pending candidates, running tests, and updating the results. It includes appropriate error handling and exit codes.
hack/release_candidates_repository.py (5)
23-29: Good YAML configuration for maintaining file structureUsing
ruamel.yamlwith these configuration options helps preserve the original file structure, which is important for maintainability, especially when these files might be edited manually.
32-56: LGTM! Well-designed ReleaseCandidate class with proper initialization.The class correctly handles optional parameters and generates a UUID if an ID isn't provided. The metadata dictionary structure follows the pattern defined in the shared types.
134-143: LGTM! Clear status update logic with appropriate conditions.The update_status method properly handles the tested_at timestamp based on whether it's provided and the new status value, with appropriate defaults.
107-132: LGTM! Well-implemented save method with duplicate detection.The save method correctly handles both updating existing candidates and adding new ones. It also includes a check to avoid adding duplicate candidates with the same components.
145-156: LGTM! Robust file reading with proper error handling.The _read_file method handles all edge cases well: non-existent files, empty files, and exceptions during reading. It returns a valid default structure in all error cases.
hack/test/test_version_discovery.py (3)
1-16: Validate fixture naming and usage.The
component_scannerfixture is well-structured, mocking the GitHub client for isolated testing. If needed, consider renaming it or adding docstrings describing its purpose for improved test clarity.
41-51: Good use of mocks to validate version-based logic.This test ensures the repository tag is derived correctly. The approach is clear and well-isolated for easy maintenance.
97-115: Excellent test for creating a new file.The new file scenario is well covered here, verifying correct creation and write operations. No issues found.
hack/version_discovery.py (6)
1-24: Logging and path configuration look solid.The initial setup for logging and path insertion appears straightforward. No concerns here.
53-69: Confirm PyGithub concurrency safety.
get_repository_commitsis called in a threaded context. PyGithub can work under concurrency, but confirm it is fully thread-safe for your usage or consider a per-thread client approach if needed.
113-129: Gracefully handling older or non-standard tags.The function returns after finding the first valid release that meets the prefix requirement. Generally, PyGithub orders them correctly, but keep in mind you may skip newer but mislabeled tags. This is likely acceptable for your use case.
169-217: Logical fallback from release-based discovery to commit-based images.The workflow is well-structured to check for releases first, then commit-based images if releases are absent. Logging is sufficiently informative for debug. Good job.
218-230: Efficient integration with ReleaseCandidatesRepository.The method seamlessly wraps the results into a
ReleaseCandidateobject. The logic for skipping if no changes are detected is a nice optimization.
232-251: Main function flow is clear.The top-level logic is straightforward: instantiate, scan, save, and exit on failure. This effectively finalizes the scanning pipeline. Well done.
hack/ansible_test_runner.py
Outdated
| image_url: str = component["image_url"] | ||
| env_var = component_env_map.get(repo_name) |
There was a problem hiding this comment.
Add null check for image_url before using it in conditional
The code accesses image_url on line 66 without checking if it might be None, which could lead to errors if a component doesn't have an image URL.
repo_name: str = repo.removeprefix("https://github.com/")
image_url: str = component["image_url"]
env_var = component_env_map.get(repo_name)
if not env_var:
logger.warning(
f"No environment variable mapping for component: {component}"
)
continue
if (
repo == "https://github.com/kubernetes-sigs/cluster-api"
or repo == "https://github.com/metal3-io/cluster-api-provider-metal3"
):
value = component["ref"]
else:
value = component["image_url"]
- if repo_name == "openshift/assisted-installer":
+ if repo_name == "openshift/assisted-installer" and image_url is not None:
if "controller" in image_url.lower():
env_var = "ASSISTED_INSTALLER_CONTROLLER_IMAGE"
else:
env_var = "ASSISTED_INSTALLER_IMAGE"Also applies to: 65-69
hack/version_discovery.py
Outdated
| logger.error("No components found") | ||
| sys.exit(1) | ||
|
|
||
| scanner.save_to_release_candidates(results) |
There was a problem hiding this comment.
Let's try to follow the single responsibility principle: ComponentScanner should only scan. Then we can save its finding through another class, for example ReleaseCandidateRepository or some other abstraction on top of it.
I think at the moment the component scanner has too much knowledge about how we deal with release candidates (for example IMO it should not know the filename where we want to save to)
| image_url: quay.io/edge-infrastructure/assisted-installer-agent:latest-cfe93a9779dea6ad2a628280b40071d23f3cb429 | ||
| - repository: https://github.com/openshift/assisted-installer | ||
| ref: c389a38405383961d26191799161c86127451635 | ||
| image_url: quay.io/edge-infrastructure/assisted-installer-controller:latest-c389a38405383961d26191799161c86127451635 |
There was a problem hiding this comment.
Thinking about image, I think we should definitely have the image repository (i.e. quay.io/edge-infrastructure/assisted-installer-controller), so that we can generalize how to search the latest image.
We might want to save the full image with digest/tag in "tested_image" or something like that
| return set1 == set2 | ||
|
|
||
|
|
||
| class ReleaseCandidatesRepository: |
There was a problem hiding this comment.
Repository pattern should only save and load to and from storage.
We might add options to retrieve filtered results, but we might want to push it to ReleaseCandidateCollection methods.
For the simplest repoistory, I envision something like this:
rcCollection = repo.list()
rc = rcCollection.find_by_status(status="pending")
new_status = run_test(rc)
rc.SetStatus(new_status)
rcCollection.upsert(rc)
# in this case, save deals with a list
repo.save(rcCollection)
for a more complex repository, we might want something like this:
opts = [FilterOption(field="status",value="pending")]
rc = repo.list(opts)[0] // check index/errors etc
new_status = run_test(rc)
rc.SetStatus(new_status)
# in this case, save deals with an item. Optionally we could take 2 params but we can infer ID by the object itself
repo.save(rc)
| else: | ||
| self.metadata["id"] = str(uuid.uuid4()) |
There was a problem hiding this comment.
nit: else is almost never necessary
if id is None:
id = str(uuid.uuid4())
self.metadata["id"] = id
hack/shared_types.py
Outdated
|
|
||
| from typing import TypedDict | ||
|
|
||
| class RepositoryInfo(TypedDict): |
There was a problem hiding this comment.
Should this be Component?
hack/version_discovery.py
Outdated
| logger.error(f"Error finding commit with image for {repo}: {e}") | ||
| raise | ||
|
|
||
| def scan_components(self) -> list[RepositoryInfo]: |
There was a problem hiding this comment.
Should this return ComponentCollection instead of list[Component]
hack/version_discovery.py
Outdated
|
|
||
|
|
||
| def main() -> None: | ||
| try: |
There was a problem hiding this comment.
ReleaseCandidatesRepository could be initialized by factory method (so we'll load filename from env)
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omer-vishlitzky The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
size/XS
https://issues.redhat.com/browse/MGMT-19400
Summary by CodeRabbit
New Features
Tests
Chores