Make datacenter field as an optional entry in vmware creds form #1256
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as off-topic.
This comment was marked as off-topic.
Signed-off-by: sarika <sarika@platform9.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (15)
- k8s/migration/pkg/utils/rollingmigrationutils.go (236-240) When a non-empty `datacenter` parameter is provided, there's no validation to ensure it exists. Consider adding validation to prevent runtime errors if an invalid datacenter name is passed.
- k8s/migration/pkg/utils/credutils.go (697-715) In the datacenter scanning loop, if all datacenters fail with errors (can't find datacenter or can't list VMs), the function will return an empty VM list rather than an error. Consider aggregating these errors and returning them if no datacenters were successfully processed to avoid silent failures.
- k8s/migration/pkg/utils/rollingmigrationutils.go (216-216) The `datacenter` parameter has been added to `GetESXiHostSystem`, but its purpose isn't documented. Consider adding a comment explaining when to use this parameter versus when to rely on the datacenter from VMware credentials.
-
ui/src/hooks/api/useVMwareMachinesQuery.ts (77-77)
Missing fallback for empty values on line 77. You should add `|| ''` to be consistent with line 72.
const vmClusterLabel = vm.metadata?.labels?.['vjailbreak.k8s.pf9.io/vmware-cluster'] || '' - k8s/migration/pkg/utils/credutils.go (1688-1690) The vmDatacenter parameter is used to construct the clusterName when no cluster is found, but there's no validation that this parameter is non-empty. Consider adding validation to handle cases where vmDatacenter might be empty to avoid potential issues with cluster naming.
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_vmwarecreds.yaml (100-100) This change correctly makes the datacenter field optional by removing it from the required fields list, which aligns with the PR title. However, you might want to consider updating the field description to indicate that it's now optional, as the current description doesn't mention this.
- ui/src/api/vmware-creds/vmwareCreds.ts (53-53) Consider using a more specific type instead of `any` for `credBody`. You could use the `VMwareCreds` type that's already imported or create a dedicated interface for this structure.
- ui/src/hooks/api/useVMwareMachinesQuery.ts (64-64) Consider using a more type-safe approach instead of casting to `any`. You could define a proper interface for the metadata that includes annotations, or use optional chaining with a type guard.
- ui/src/hooks/api/useVMwareMachinesQuery.ts (72-72) There's inconsistent handling of empty values. On line 65, you use `|| ''` for annotations, but on line 72 you use `|| ''` for the label. Consider extracting this pattern into a helper function for consistency.
-
ui/src/features/migration/MigrationForm.tsx (307-308)
Same conditional spread pattern here. Consider using a more explicit approach for better readability:
const templateParams = { vmwareRef: vmwareCredentials?.metadata.name, openstackRef: openstackCredentials?.metadata.name, targetPCDClusterName, useFlavorless: params.useFlavorless || false, useGPUFlavor: params.useGPU || false }; if (params.vmwareCreds?.datacenter) { templateParams.datacenter = params.vmwareCreds.datacenter; } const body = createMigrationTemplateJson(templateParams); - ui/src/features/migration/SourceDestinationClusterSelection.tsx (190-196) Consider using a consistent pattern for fallback values. In the renderValue function, you're using `cluster?.displayName || cluster?.name || 'Unknown Cluster'`, but in other parts of the code you might be using just displayName. Ensure consistency across the codebase for better maintainability.
-
ui/src/features/migration/MigrationForm.tsx (285-287)
This conditional spread pattern (`...(condition && { property: value })`) works but can be hard to read. Consider using a more explicit approach for conditional properties:
const sourceSpec = {}; if (params.vmwareCreds?.datacenter) { sourceSpec.datacenter = params.vmwareCreds.datacenter; } sourceSpec.vmwareRef = vmwareCredentials?.metadata.name; const patchBody = { spec: { source: sourceSpec, - ui/src/hooks/api/useVMwareMachinesQuery.ts (57-59) There are unnecessary empty lines at lines 57 and 59 that should be removed for consistent code style.
-
ui/src/api/vmware-creds/vmwareCreds.ts (68-68)
The comment could be more precise. It's not just handling empty strings but also undefined/null values:
// Use empty string when datacenter is undefined, null, or empty credBody.spec.datacenter = datacenter?.trim() || '' - k8s/migration/api/v1alpha1/vmwarecreds_types.go (41-41) I notice there's an inconsistency in naming between `DataCenter` in VMwareCredsSpec and `Datacenter` in VMwareCredsInfo. Consider standardizing the casing across both structs to maintain consistency and avoid potential confusion when mapping between these structures.
💡 To request another review, post a new comment with "/windsurf-review".
Contributor
|
Bito Review Skipped - No Changes Detected |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Which issue(s) this PR fixes
fixes #404
Testing done
Summary by Bito