-
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
Helm related fixes #338
Helm related fixes #338
Conversation
Reviewer's Guide by SourceryThis PR implements several Helm-related improvements and fixes, focusing on helm chart management, service interception, and secret handling. The changes include fixing helm fetch functionality for manual URL entries, adding Helm management tags to secrets, and updating port variables for service interception. ER diagram for Secret management changeserDiagram
SECRET {
string displayName
boolean createdByHelm
}
SECRET ||--o{ RESOURCE_EXTRA_ACTION : "managed by"
RESOURCE_EXTRA_ACTION {
boolean disabled
}
Class diagram for Helm-related changesclassDiagram
class HelmChartLayout {
- selectedRepo: string
+ packageId: string
- activeTab: string = 'defaults'
+ activeTab: string = 'values'
}
class SecretResourceV2 {
+ createdByHelm: boolean
}
class InterceptPortView {
- containerPort
+ devicePort
}
class ServiceBindingQueries {
- containerPort
+ devicePort
}
class AppQueries {
+ protocol: string
}
class SecretQueries {
+ createdByHelm: boolean
}
class ExposedPortList {
+ protocol: string = 'TCP'
}
HelmChartLayout --> SecretResourceV2 : uses
InterceptPortView --> ServiceBindingQueries : uses
AppQueries --> SecretQueries : uses
ExposedPortList --> InterceptPortView : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @tulsiojha - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -396,8 +397,9 @@ const RenderField = ({ | |||
if (field.type === 'int-range') { |
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.
issue (complexity): Consider extracting shared number range input logic into a reusable component to reduce code duplication
The int-range and Resource field types contain significant duplication and unnecessary nesting. Consider:
- Extract shared number range logic into a reusable component:
const NumberRangeInput = ({
field,
value,
onChange,
errors,
fieldKey
}) => {
const formatValue = (val, unit, multiplier = 1) =>
`${parseFloat(val) * multiplier}${unit}`;
return (
<div className="flex flex-row gap-xl items-end flex-1">
<NumberInput
size="lg"
error={!!errors[`${fieldKey}.min`]}
message={errors[`${fieldKey}.min`]}
placeholder={`${field.label} min`}
value={parseFloat(value.min) / (field.multiplier || 1)}
onChange={({target}) => {
onChange(`${field.input}.min`)(
formatValue(target.value, field.unit, field.multiplier)
);
}}
suffix={field.displayUnit}
/>
<NumberInput
// Similar props for max input
/>
</div>
);
};
- Simplify the field type handling:
if (field.type === 'int-range' || field.type === 'Resource') {
return (
<div className="flex flex-col gap-md">
<Label required={field.required}>{field.label}</Label>
<div className="flex flex-row gap-xl items-center">
<NumberRangeInput
field={field}
value={value}
onChange={onChange}
errors={errors}
fieldKey={fieldKey}
/>
{field.type === 'Resource' && (
<QosSwitch
checked={qos}
onChange={handleQosChange}
/>
)}
</div>
</div>
);
}
This reduces nesting, eliminates the dummyEvent wrapper, and makes the code more maintainable while preserving all functionality.
Summary by Sourcery
Fix Helm fetch for manual URL entries, add 'managed by Helm' tag to secrets, and update intercept port variables for improved functionality and resource management.
Bug Fixes:
Enhancements: