Create ssh public key field - #1180
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tmstff 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 change adds ARO HCP node SSH key support and AWS Spot instance configuration to the models, OpenAPI schemas, generated Go client, builders, and JSON handling. It also updates release metadata and makes model-file lint discovery explicit. ChangesARO HCP cluster SSH key
AWS Spot instance configuration
Supporting updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AWSNodePoolBuilder
participant AwsNodePoolSpotMarketOptionsBuilder
participant JSONSerializer
Client->>AWSNodePoolBuilder: set SpotMarketOptions
AWSNodePoolBuilder->>AwsNodePoolSpotMarketOptionsBuilder: build nested options
AWSNodePoolBuilder-->>Client: return AWSNodePool
Client->>JSONSerializer: serialize spot_market_options
JSONSerializer-->>Client: return JSON payload
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
model/aro_hcp/v1alpha1/cluster_type.model (1)
260-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGenerated doc comments say
'node_SSH_public_key'instead of the actualnode_ssh_public_keyJSON key.The model field name
NodeSSHPublicKey(consecutive caps "SSH") causes the code generator's snake_case derivation used for Godoc text to preserve the capital "SSH", while the real JSON/OpenAPI key is correctly all-lowercasenode_ssh_public_key. Purely cosmetic (doc text only, wire format unaffected), but confusing to SDK consumers reading the generated docs.
model/aro_hcp/v1alpha1/cluster_type.model#L260-L265: consider renaming the field (e.g.NodeSshPublicKey) if the generator's acronym handling can't be special-cased, to avoid the incorrect derived doc string.clientapi/arohcp/v1alpha1/cluster_type.go#L1290-L1317:NodeSSHPublicKey()/GetNodeSSHPublicKey()doc comments reference'node_SSH_public_key'; would be regenerated automatically once the root cause is addressed.clientapi/arohcp/v1alpha1/cluster_builder.go#L775-L783:NodeSSHPublicKeysetter doc comment has the same mismatch; would be regenerated automatically once the root cause is addressed.🤖 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 `@model/aro_hcp/v1alpha1/cluster_type.model` around lines 260 - 265, Rename the model field NodeSSHPublicKey to NodeSshPublicKey in model/aro_hcp/v1alpha1/cluster_type.model lines 260-265 so generated documentation derives the correct node_ssh_public_key spelling, while preserving the JSON/OpenAPI key and API behavior; regenerate the affected client code, including clientapi/arohcp/v1alpha1/cluster_type.go lines 1290-1317 and clientapi/arohcp/v1alpha1/cluster_builder.go lines 775-783, with no direct manual changes required there beyond the generated rename and corrected comments.
🤖 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.
Nitpick comments:
In `@model/aro_hcp/v1alpha1/cluster_type.model`:
- Around line 260-265: Rename the model field NodeSSHPublicKey to
NodeSshPublicKey in model/aro_hcp/v1alpha1/cluster_type.model lines 260-265 so
generated documentation derives the correct node_ssh_public_key spelling, while
preserving the JSON/OpenAPI key and API behavior; regenerate the affected client
code, including clientapi/arohcp/v1alpha1/cluster_type.go lines 1290-1317 and
clientapi/arohcp/v1alpha1/cluster_builder.go lines 775-783, with no direct
manual changes required there beyond the generated rename and corrected
comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1ef1049-26b4-4cdc-945a-b13395dd9d0c
📒 Files selected for processing (6)
Makefileclientapi/arohcp/v1alpha1/cluster_builder.goclientapi/arohcp/v1alpha1/cluster_type.goclientapi/arohcp/v1alpha1/cluster_type_json.gomodel/aro_hcp/v1alpha1/cluster_type.modelopenapi/aro_hcp/v1alpha1/openapi.json
| // The SSH public key used for secure access to cluster nodes. When set, this key | ||
| // is distributed to all nodes, enabling SSH access for debugging | ||
| // and maintenance purposes. | ||
| // This is currently only supported for ARO HCP |
There was a problem hiding this comment.
// This is currently only supported for ARO HCP
This comment is not needed, as the model is specific to aro-hcp
| // is distributed to all nodes, enabling SSH access for debugging | ||
| // and maintenance purposes. | ||
| // This is currently only supported for ARO HCP | ||
| NodeSSHPublicKey String |
There was a problem hiding this comment.
Is it updatable? If not, document it in the same way as other fields that are not.
There was a problem hiding this comment.
If it's updatable, are both directions supported? document any restrictions
There was a problem hiding this comment.
It's updatable. What do you mean by "both directions" ?
There was a problem hiding this comment.
by both directions I mean:
- update from "there's a key set" to "no key set"
- update from "no key set" to "there's a key set"
There was a problem hiding this comment.
It's updatable.
Do we have an understanding on how that would be achieved (at RP level, CS level, ...) and what would be the impact (recreation of nodepools vs not, etc...) ? this is a cluster-level attribute but it controls node pool related configuration. This would include figuring out how to perform update state calculation and so on.
Do we know whether Hypershift supports updating it? if so, what are the consequences of doing so.
| // is distributed to all nodes, enabling SSH access for debugging | ||
| // and maintenance purposes. | ||
| // This is currently only supported for ARO HCP | ||
| NodeSSHPublicKey String |
There was a problem hiding this comment.
What is the expected format? explain it
There was a problem hiding this comment.
valid ssh public key. Can add that.
| "description": "Network settings of the cluster.", | ||
| "$ref": "#/components/schemas/Network" | ||
| }, | ||
| "node_ssh_public_key": { |
There was a problem hiding this comment.
Change to plural: nodes_ssh_public_key
| "$ref": "#/components/schemas/Network" | ||
| }, | ||
| "node_ssh_public_key": { | ||
| "description": "The SSH public key used for secure access to cluster nodes. When set, this key\nis distributed to all nodes, enabling SSH access for debugging\nand maintenance purposes.\nThis is currently only supported for ARO HCP", |
There was a problem hiding this comment.
cluster nodes -> cluster's node pool's nodes
| // This is currently only supported for ROSA HCP | ||
| AutoNode ClusterAutoNode | ||
|
|
||
| // The SSH public key used for secure access to cluster nodes. When set, this key |
There was a problem hiding this comment.
| // This is currently only supported for ROSA HCP | ||
| AutoNode ClusterAutoNode | ||
|
|
||
| // The SSH public key used for secure access to cluster nodes. When set, this key |
There was a problem hiding this comment.
Has the API design been reviewed and approved on ARM side?
There was a problem hiding this comment.
The ARM side PR is Azure/ARO-HCP#6157, which is currently in draft. I left some comments there too
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGES.md`:
- Around line 6-8: Remove the duplicate ROSAENG-61032 entries from CHANGES.md,
keeping a single entry for the change or replacing the duplicates with distinct
release 0.0.462 changes.
🪄 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-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e49f56fb-662a-4880-b11b-93d128607f60
📒 Files selected for processing (16)
CHANGES.mdclientapi/clustersmgmt/v1/aws_builder.goclientapi/clustersmgmt/v1/aws_node_pool_builder.goclientapi/clustersmgmt/v1/aws_node_pool_spot_market_options_builder.goclientapi/clustersmgmt/v1/aws_node_pool_spot_market_options_list_builder.goclientapi/clustersmgmt/v1/aws_node_pool_spot_market_options_list_type_json.goclientapi/clustersmgmt/v1/aws_node_pool_spot_market_options_type.goclientapi/clustersmgmt/v1/aws_node_pool_spot_market_options_type_json.goclientapi/clustersmgmt/v1/aws_node_pool_type.goclientapi/clustersmgmt/v1/aws_node_pool_type_json.goclientapi/clustersmgmt/v1/aws_type.goclientapi/clustersmgmt/v1/aws_type_json.gomodel/clusters_mgmt/v1/aws_node_pool_spot_market_options_type.modelmodel/clusters_mgmt/v1/aws_node_pool_type.modelmodel/clusters_mgmt/v1/aws_type.modelopenapi/clusters_mgmt/v1/openapi.json
| - ROSAENG-61032 | task: ocm-api/sdk changes to support SpotMarketOptions for ROSA HCP | ||
| - ROSAENG-61032 | task: ocm-api/sdk changes to support SpotMarketOptions for ROSA HCP | ||
| - ROSAENG-61032 | task: ocm-api/sdk changes to support SpotMarketOptions for ROSA HCP |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate changelog entries.
Lines 6-8 contain identical text. Keep one entry, or replace the duplicates with distinct changes included in release 0.0.462.
🤖 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 `@CHANGES.md` around lines 6 - 8, Remove the duplicate ROSAENG-61032 entries
from CHANGES.md, keeping a single entry for the change or replacing the
duplicates with distinct release 0.0.462 changes.
3 participants
Description
Implementation of ARO-26937: Update OCM SDK to include SSH public key field :
As part of the effort to make it possible for ARO HCP customers to connect to a cluster node via ssh, this change adds the fiels nodeSshPublicKey to enable the ARO HCP RP Backend to propagate the key to CS (which can then forward it to HyperShift).
Type of Change
make updateafter model change)Verification
make checkpasses (model syntax validation)make verifypasses (generated code matches model)make lintpasses (indentation enforcement) -> did not pass, but also on >4.000 locations that were not touched my this PR. Will try to take care of this separately.Checklist
clientapi/andopenapi/are updated if model changed (make update)Summary by CodeRabbit
New Features
Chores