Skip to content

Add rack-types #3026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

bastianleicht
Copy link
Contributor

@bastianleicht bastianleicht commented Apr 18, 2025

As described in #2618

Implementation of the importer in: netbox-community/Device-Type-Library-Import#170

@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 13:36
@bastianleicht bastianleicht changed the title Add rack-ypes Add rack-types Apr 18, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new rack type definition for a Startech rack and updates the front image setting for an HPE device type, addressing issue #2618 and supporting the related Device-Type-Library-Import PR.

  • Added a new YAML file for the Startech 4 Post 42U rack with key specifications.
  • Updated the HPE OfficeConnect device type to set front_image to true.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
rack-types/Startech/startech-4postrack42.yaml Introduces a new rack type with specifications and includes commented alternatives for mounting depth and weight.
device-types/HPE/OfficeConnect-1920-24G-PoE-180W.yml Changes the front_image property from false to true.
Comments suppressed due to low confidence (1)

device-types/HPE/OfficeConnect-1920-24G-PoE-180W.yml:8

  • Verify that setting front_image to true is the intended change, and if so, ensure that associated tests and documentation reflect this update.
front_image: true

Comment on lines +12 to +16
# Adjustable depth, do we want the minimum or maximum depth?
# Minimum adjusted depth
mounting_depth: 560
# Maximum adjusted depth
# mounting_depth: 1017
Copy link
Preview

Copilot AI Apr 18, 2025

Choose a reason for hiding this comment

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

Consider clarifying or removing the commented out mounting_depth values to avoid potential confusion about which depth should be applied.

Suggested change
# Adjustable depth, do we want the minimum or maximum depth?
# Minimum adjusted depth
mounting_depth: 560
# Maximum adjusted depth
# mounting_depth: 1017
# Adjustable depth: using the minimum adjusted depth
mounting_depth: 560

Copilot uses AI. Check for mistakes.

Comment on lines +12 to +21
# Adjustable depth, do we want the minimum or maximum depth?
# Minimum adjusted depth
mounting_depth: 560
# Maximum adjusted depth
# mounting_depth: 1017
weight: 38.5
# Different weights between stationary and on casters, which one?
# Stationary
# max_weight: 600
# Rolling
Copy link
Preview

Copilot AI Apr 18, 2025

Choose a reason for hiding this comment

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

Clarify which weight value is intended for use in production or remove the extra commented weight values if they are outdated.

Suggested change
# Adjustable depth, do we want the minimum or maximum depth?
# Minimum adjusted depth
mounting_depth: 560
# Maximum adjusted depth
# mounting_depth: 1017
weight: 38.5
# Different weights between stationary and on casters, which one?
# Stationary
# max_weight: 600
# Rolling
# Adjustable depth: using minimum adjusted depth for production.
mounting_depth: 560
weight: 38.5
# Maximum weight capacity when rolling (on casters).

Copilot uses AI. Check for mistakes.

@harryajc
Copy link
Collaborator

Hi @bastianleicht Thanks for the contribution,
We also need to add the pre commit checks for the rack device types.

@bastianleicht
Copy link
Contributor Author

@harryajc The Pre-Commit Checks should already work? As far as I know, they are not limited to folders etc.

Or am I wrong here?

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