Skip to content

Conversation

@amaslenn
Copy link
Contributor

@amaslenn amaslenn commented Jan 20, 2026

Summary

File wasn't handled for is_installed_one and mark_as_installed_one of KubernetesInstaller which led to unnecessary re-installation.

It adresses internal issue.

Test Plan

  1. CI (extended)
  2. Manual check.

Additional Notes

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

KubernetesInstaller now handles File items in installation checks and marking; tests were changed to always include a File item for all installers. A file header copyright year was bumped.

Changes

Cohort / File(s) Summary
KubernetesInstaller File support
src/cloudai/systems/kubernetes/kubernetes_installer.py
is_installed_one added support for File items: verifies target file exists at system_install_path and that contents match the source, returning success when matching and failure with "File {path} does not exist." when absent. mark_as_installed_one added handling for File items by setting item.installed_path to system_install_path / item.src.name.
Tests: include File for all installers
tests/test_base_installer.py
test_check_supported now always includes File(Path(__file__)) for every installer (removed previous conditional for SlurmInstaller).
Metadata/header update
src/cloudai/systems/kubernetes/kubernetes_installer.py
Copyright year range updated from 2024-2025 to 2024-2026.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I checked the paths with careful hops,
Matched file bytes and tied loose props,
Tests now hop the same bright tune,
A tiny patch beneath the moon,
Hooray — I left a carrot & some docs.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix installation logic for File on k8s' clearly and specifically identifies the main change: fixing File handling in KubernetesInstaller's installation logic.
Description check ✅ Passed The pull request description accurately describes the changes: fixing File handling in KubernetesInstaller's is_installed_one and mark_as_installed_one methods to prevent unnecessary re-installation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/cloudai/systems/kubernetes/kubernetes_installer.py (1)

2-2: Update copyright year to 2026.

The pipeline failure indicates the SPDX header year is outdated.

Proposed fix
-# Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
tests/test_base_installer.py (1)

2-2: Update copyright year to 2026.

The pipeline failure indicates the SPDX header year is outdated.

Proposed fix
-# Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
🤖 Fix all issues with AI agents
In `@src/cloudai/systems/kubernetes/kubernetes_installer.py`:
- Around line 121-124: The check for truthiness on File.installed_path is
redundant because File.installed_path always returns a Path; update the
kubernetes_installer.py branch that handles File items so it directly checks
existence via item.installed_path.exists() and returns InstallStatusResult(True)
when it exists, otherwise return InstallStatusResult(False, f"File
{item.installed_path} does not exist."), keeping the same InstallStatusResult
usage and message.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 20, 2026

Greptile Overview

Greptile Summary

Added missing File handling to KubernetesInstaller.is_installed_one() and KubernetesInstaller.mark_as_installed_one() methods, aligning implementation with SlurmInstaller.

  • Added File type check in is_installed_one() that verifies file existence at install location and compares contents
  • Added File type check in mark_as_installed_one() that sets the installed path
  • Updated test to verify File support for both Kubernetes and Slurm installers
  • Copyright year updated to 2026

This fix prevents unnecessary re-installation of files by properly tracking their installation status.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are straightforward and align the KubernetesInstaller implementation with the existing SlurmInstaller. The implementation properly handles File type checking, content validation, and path tracking. Previous review comments have been addressed, and tests have been updated to verify both installers support File objects.
  • No files require special attention

Sequence Diagram

sequenceDiagram
    participant Client
    participant KubernetesInstaller
    participant File
    participant FileSystem

    Note over Client,FileSystem: File Installation Check Flow

    Client->>KubernetesInstaller: is_installed_one(File)
    KubernetesInstaller->>FileSystem: Check if install_path/file exists
    alt File exists
        FileSystem-->>KubernetesInstaller: exists=true
        KubernetesInstaller->>FileSystem: Read installed file contents
        FileSystem-->>KubernetesInstaller: installed_contents
        KubernetesInstaller->>FileSystem: Read source file contents
        FileSystem-->>KubernetesInstaller: source_contents
        KubernetesInstaller->>KubernetesInstaller: Compare contents
        alt Contents match
            KubernetesInstaller->>File: Set installed_path
            KubernetesInstaller-->>Client: InstallStatusResult(True)
        else Contents differ
            KubernetesInstaller-->>Client: InstallStatusResult(False, "File does not exist")
        end
    else File does not exist
        FileSystem-->>KubernetesInstaller: exists=false
        KubernetesInstaller-->>Client: InstallStatusResult(False, "File does not exist")
    end

    Note over Client,FileSystem: Mark as Installed Flow

    Client->>KubernetesInstaller: mark_as_installed_one(File)
    KubernetesInstaller->>File: Set installed_path = install_path/src.name
    KubernetesInstaller-->>Client: InstallStatusResult(True)
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/cloudai/systems/kubernetes/kubernetes_installer.py`:
- Around line 121-127: The current File handling in kubernetes_installer.py uses
read_text() which fails on binary files and yields wrong error wording; change
the logic in the branch that handles isinstance(item, File) to compare bytes
using (self.system.install_path / item.src.name).read_bytes() and
item.src.read_bytes(), set item.installed_path when bytes match and return
InstallStatusResult(True), and when the file exists but bytes differ return
InstallStatusResult(False, f"File {self.system.install_path / item.src.name}
exists but content differs") instead of saying it does not exist; ensure you
still return a not-found message if the file truly doesn't exist.

Copy link
Contributor

@jeffnvidia jeffnvidia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@amaslenn amaslenn merged commit fa1428b into main Jan 20, 2026
5 checks passed
@amaslenn amaslenn deleted the am/bug-4843478 branch January 20, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants