Skip to content

Conversation

CrAazZyMaN21
Copy link

Solves #1553

Also adding CentOS CoreOS for the OKD project 😃

@CrAazZyMaN21 CrAazZyMaN21 force-pushed the development branch 2 times, most recently from 9985e79 to 11ef6a7 Compare September 12, 2025 20:45
@CrAazZyMaN21 CrAazZyMaN21 marked this pull request as draft September 12, 2025 20:52
@CrAazZyMaN21 CrAazZyMaN21 force-pushed the development branch 2 times, most recently from 8ff37bc to 9bac5d2 Compare September 12, 2025 20:59
@CrAazZyMaN21 CrAazZyMaN21 marked this pull request as ready for review September 12, 2025 21:05
@antonym
Copy link
Member

antonym commented Sep 12, 2025

@claude review

Copy link
Contributor

claude bot commented Sep 12, 2025

Claude finished @antonym's task —— View job


Code Review: Add CentOS CoreOS & Red Hat Linux CoreOS

I've reviewed PR #1674 which adds support for CentOS CoreOS and Red Hat Linux CoreOS. Here's my comprehensive analysis:

Positive Aspects

  1. Addresses Valid Need: The PR solves issue [Request] RedHat CoreOS (RHCOS) for OpenShift PXE Boot #1553 and adds support for two important CoreOS variants used in enterprise environments
  2. Follows Existing Patterns: The implementation appears to follow the established pattern used by Fedora CoreOS
  3. Appropriate File Structure: New template files follow the naming convention (centos-coreos.ipxe.j2, rhcos.ipxe.j2)

⚠️ Areas for Review & Potential Issues

1. Configuration Structure Concerns
Based on the existing CoreOS configuration in roles/netbootxyz/defaults/main.yml:191-203, I notice:

  • The Fedora CoreOS configuration has specific base_dir, mirror, and versioning structure
  • Need to ensure CentOS CoreOS and RHCOS have appropriate configuration entries with:
    • Proper mirror URLs and base directories
    • Correct versioning scheme
    • Appropriate naming conventions

2. Template File Consistency
From examining the existing coreos.ipxe.j2 template, the new templates should:

  • Follow the same parameter structure (install_device, ignition_url)
  • Use consistent variable naming and menu structure
  • Handle architecture detection properly (x86_64 vs aarch64)
  • Include proper error handling and exit paths

3. README Integration
The README.md should be updated to include:

  • CentOS CoreOS entry in the Operating Systems table (around line 138 where Fedora CoreOS is listed)
  • Red Hat CoreOS entry with appropriate URL references
  • Consistent formatting with existing entries

4. Missing Configuration Verification
Since I cannot access the actual PR files due to branch access limitations, I recommend verifying:

For main.yml additions:

releases:
  centos-coreos:
    base_dir: [appropriate path]
    enabled: true
    menu: linux
    mirror: [official CentOS CoreOS mirror]
    name: CentOS CoreOS
    versions:
    - code_name: [version]
      name: [display name]
  
  rhcos:
    base_dir: [appropriate path] 
    enabled: true
    menu: linux
    mirror: [official RHCOS mirror]
    name: Red Hat CoreOS
    versions:
    - code_name: [version]
      name: [display name]

For template files:

  • Ensure URL patterns match official distribution methods
  • Verify kernel/initrd/rootfs URL structures
  • Test ignition config integration
  • Validate install device handling

🔍 Recommendations

  1. Verify Mirror URLs: Ensure the mirror URLs point to official, stable sources
  2. Test Boot Process: Validate that the boot process works with real hardware/VMs
  3. Documentation: Consider adding usage examples or links to official documentation
  4. Versioning Strategy: Ensure the versioning scheme aligns with upstream release patterns

📝 Final Assessment

The PR appears to be a solid addition that follows established patterns. However, without access to the actual implementation details, I recommend:

  1. Manual testing of the boot sequences
  2. Verification of mirror URL stability and availability
  3. Ensuring the configuration aligns with official CoreOS installation methods

Once these items are confirmed, this should be a valuable addition to netboot.xyz's distribution support.


@CrAazZyMaN21
Copy link
Author

Hi, I tried to find the issue why the Build PR Pipeline failed but I can't seem to find it. I am guessing the file name "centos-coreos.ipxe.j2" is not suitable and should be changed to e.g "scos.ipxe.j2"

@CrAazZyMaN21 CrAazZyMaN21 changed the title Add CentOS CoreOS & Red Hat Linux CoreOS Add CentOS CoreOS (SCOS) & Red Hat Linux CoreOS (RHCOS) Sep 15, 2025
@CrAazZyMaN21
Copy link
Author

@antonym Can you please let the workflow run again. I think it is fixed now and I squashed the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants