-
Notifications
You must be signed in to change notification settings - Fork 1
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
Impl/app image pull #145
Impl/app image pull #145
Conversation
compute and scaling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nxtcoder36 - I've reviewed your changes and they look great!
General suggestions:
- Ensure that changes to the image pull policy are clearly communicated to the user, highlighting the implications for deployment behavior.
- Verify that the removal of default array fallbacks in environment variable initialization does not introduce runtime errors.
- Maintain UI consistency across different settings pages, especially with the addition of new controls like checkboxes.
- Consider the broader impact of these changes on the application's deployment and scaling behavior, ensuring they align with user expectations.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
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? ✨
@@ -125,55 +125,56 @@ const SettingCompute = () => { | |||
max={10} | |||
value={[values.minReplicas, values.maxReplicas]} | |||
onChange={(value) => { | |||
console.log(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Removal of the console.log statement is a good practice for production code to avoid exposing potentially sensitive information and to keep the console clean. Ensure that all necessary debugging has been completed.
label="Image Pull Policy" | ||
checked={values.imagePullPolicy === 'Always'} | ||
onChange={(val) => { | ||
const imagePullPolicy = val ? 'Always' : 'IfNotPresent'; | ||
console.log(imagePullPolicy); | ||
handleChange('imagePullPolicy')(dummyEvent(imagePullPolicy)); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_clarification): The implementation of the 'Image Pull Policy' checkbox is a good addition for user flexibility. However, ensure that the UI clearly communicates the implications of changing this setting to the user, as it can affect the deployment's behavior regarding image updates.
@@ -230,7 +230,7 @@ export const EnvironmentVariables = () => { | |||
submit, | |||
resetValues: resetAppValue, | |||
} = useForm({ | |||
initialValues: getContainer().env || [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (edge_case_not_handled): This change simplifies the initialization of environment variables by assuming that getContainer().env
will always return a valid array. Ensure that the getContainer
function or its callers are robust against undefined or null values to prevent runtime errors.
@@ -261,6 +264,15 @@ const SettingCompute = () => { | |||
} | |||
loading={digestLoading} | |||
/> | |||
|
|||
<Checkbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consistency in the UI is key for a good user experience. The addition of the 'Image Pull Policy' checkbox here, similar to its addition in the new-app compute settings, helps maintain that consistency. Just ensure that the default state and any state changes are correctly managed across different components and routes.
No description provided.