Update Release #124
Update Release #124
Conversation
KLO-153: Impl/autoscaling apps
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey abdheshnayak - Here's my review!
General suggestions:
- Consider adding more descriptive comments or documentation around the new scaling feature to aid future maintainability.
- Review the consistency of formatting and indentation across the changes to ensure code quality.
- Ensure that the new handling of domain links (using onClick instead of Link component) maintains accessibility standards.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -0,0 +1,201 @@ | |||
| import { useEffect } from 'react'; | |||
There was a problem hiding this comment.
issue (llm): I noticed that this pull request introduces a new feature for scaling settings, including autoscaling configurations. However, I couldn't find any unit or integration tests related to these changes. It's crucial to ensure that the scaling settings work as expected under various conditions, including edge cases such as minimum and maximum replicas, CPU, and memory thresholds. Could you add tests to cover these scenarios?
| @@ -127,45 +127,39 @@ const SettingCompute = () => { | |||
| ...(s.spec.containers?.[0] || {}), | |||
| image: | |||
| values.repoAccountName === undefined || | |||
| values.repoAccountName === '' | |||
| values.repoAccountName === '' | |||
There was a problem hiding this comment.
issue (llm): The changes in the compute settings seem to involve adjustments in how resource limits are set. It's important to validate these changes with tests, especially focusing on the correct application of resource limits based on the selection mode (quick or manual). Please ensure there are tests that verify the logic for setting resourceCpu and resourceMemory based on different user inputs.
| @@ -7,15 +7,15 @@ import Yup from '~/root/lib/server/helpers/yup'; | |||
| import { FadeIn, parseValue } from '~/console/page-components/util'; | |||
| import Select from '~/components/atoms/select'; | |||
| import ExtendedFilledTab from '~/console/components/extended-filled-tab'; | |||
| import {parseName, parseNodes} from '~/console/server/r-utils/common'; | |||
| import { parseName, parseNodes } from '~/console/server/r-utils/common'; | |||
There was a problem hiding this comment.
issue (llm): This file has been modified to include changes in the app compute settings. Given the nature of these changes, it would be beneficial to have tests that cover the new logic introduced, particularly the interaction with the parseName and parseNodes utilities, and how they affect the app's compute configuration. Ensuring that the node pool selection and resource allocation work as intended is crucial.
| size="sm" | ||
| content={formatDomain(firstDomain).short} | ||
| variant="primary-plain" | ||
| to={formatDomain(firstDomain).full} | ||
| onClick={(e) => { |
There was a problem hiding this comment.
issue (llm): The modifications in this file seem to change how domain links are handled, moving from a Link component to an onClick event that opens the link in a new tab. While this change might seem minor, it's important to verify through tests that the expected behavior occurs without unintended side effects, especially in terms of user experience and security (e.g., ensuring noopener and noreferrer are used correctly).
| @@ -24,6 +24,7 @@ import { IAppContext } from '../_layout'; | |||
| const navItems = [ | |||
| { label: 'General', value: 'general' }, | |||
| { label: 'Compute', value: 'compute' }, | |||
| { label: 'Scaling', value: 'scaling' }, | |||
There was a problem hiding this comment.
suggestion (llm): The addition of a new 'Scaling' settings tab is a significant change. It's important to ensure that navigating to this new tab works as expected and that the UI renders correctly. Integration or end-to-end tests that simulate user interaction with this new tab would be valuable to confirm its functionality.
No description provided.