Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable SectionDetail component to display key-value configuration properties, which is integrated into the CreateRuntimeProfileComponent to render expandable sections for runtime environment, executor/driver, autoscaling, metastore, network security, session lifecycle, and other customizations. It also cleans up deprecated properties and adds comprehensive unit tests. The review feedback suggests replacing read-only useState hooks with useMemo in CreateRuntimeProfileComponent to prevent stale state bugs when props change, and adding aria-disabled to the edit button in SectionDetail to improve accessibility.
|
please address the gemini comments |
06b35aa to
707b8d4
Compare
723c33e to
707b8d4
Compare
707b8d4 to
8a03868
Compare
8a03868 to
fb30529
Compare
| customer_managed_key: 'Customer-managed key' | ||
| }; | ||
|
|
||
| export const DEFAULT_RUNTIME_ENVIRONMENT_CONFIG: IRuntimeEnvironmentConfig = { |
There was a problem hiding this comment.
Many of the values here and below should come from the API. Why are we hard-coding random values?
There was a problem hiding this comment.
When creating a new Runtime Profile, the initial summary rows ((RESOURCE_ALLOCATION_DEFAULT, AUTO_SCALING_DEFAULT, runtimeOptions = '2.3', selectedAccountRadio = 'serviceAccount', selectedEncryptionRadio = 'googleManaged') show the Serverless Spark default settings before user customization.
The dynamic resource options—specifically VPC Networks & Subnetworks / Shared VPC (listNetworksAPIService / listSubNetworksAPIService), Cloud KMS KeyRings & Keys (listKeyRingsAPIService / listKeysAPIService), Dataproc Metastore services (listMetaStoreAPIService), and Cloud Storage Staging Buckets—are fetched from the GCP APIs inside the respective Edit Drawers and API integration PRs that build on top of this static UI PR.
There was a problem hiding this comment.
So you expect to send "Name of the runtime profile" to the BE as the default runtimeProfileId? Whatever the user doesn't specify will be sent as these default strings?
| driverAndExecutorConfiguration: | ||
| payload.driverAndExecutorConfiguration ?? | ||
| payload.executorAndDriverConfig, | ||
| executorAndDriverConfig: payload.executorAndDriverConfig, |
There was a problem hiding this comment.
None of the tests are affected by these changes?
| expect(renderedText).toContain('gs://my-staging-bucket'); | ||
| expect(renderedText).toContain('Premium'); | ||
| expect(renderedText).toContain('highmem-8'); | ||
| expect(renderedText).toContain('Disabled'); |
There was a problem hiding this comment.
This will pass if there's any field that has 'Disabled' in it. These tests should check their respective fields
3 participants
This PR integrates the Additional configuration sections into the Create Runtime Profile form based on the Project Ignite specification, consuming the reusable
SectionDetailcomponent introduced in Part 1 (#457).Key Changes
CreateRuntimeProfileComponentdisplaying default configuration values across 7 sections:RuntimeEnvironmentSection)ExecutorAndDriverSection)AutoscalingSection)MetastoreSection)NetworkSecuritySection)SessionLifecycleSection)OtherCustomizationSectionfor Spark properties and Labels)createRuntimeProfile.spec.tsxcovering all section components and property formatters.Screencast / Demo
additional-config.mov
Notes for Reviewers
isEditDisabled={true}); section edit drawers and live form submission will be wired up in upcoming PRs.