Add support for generic dynamic log links #5066
Conversation
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
dosubot
bot
added
size:L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5066 +/- ##
==========================================
+ Coverage 59.00% 59.01% +0.01%
==========================================
Files 645 645
Lines 55578 55622 +44
==========================================
+ Hits 32792 32826 +34
- Misses 20194 20201 +7
- Partials 2592 2595 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/review |
Code Review Agent Run #51871aActionable Suggestions - 8
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| &core.TaskTemplate{ | ||
| Config: map[string]string{ | ||
| "link_type": "abc,defg", | ||
| "generic_dynamic_log_links": `{"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"},{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]}`, |
There was a problem hiding this comment.
Consider splitting the long line containing generic_dynamic_log_links configuration into multiple lines for better readability. The current line exceeds 120 characters which may impact code maintainability.
Code suggestion
Check the AI-generated fix before applying
| "generic_dynamic_log_links": `{"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"},{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]}`, | |
| "generic_dynamic_log_links": `{ | |
| "links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"}, | |
| {"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}] | |
| }`, |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| t.Run("Test_kubeRayCrdVersion", func(t *testing.T) { | ||
|
|
||
| t.Run("Override", func(t *testing.T) { | ||
| testValue := "1" | ||
|
|
||
| cmdFlags.Set("kubeRayCrdVersion", testValue) | ||
| if vString, err := cmdFlags.GetString("kubeRayCrdVersion"); err == nil { | ||
| testDecodeJson_Config(t, fmt.Sprintf("%v", vString), &actual.KubeRayCrdVersion) | ||
|
|
||
| } else { | ||
| assert.FailNow(t, err.Error()) | ||
| } | ||
| }) | ||
| }) | ||
| t.Run("Test_serviceAccount", func(t *testing.T) { | ||
|
|
||
| t.Run("Override", func(t *testing.T) { | ||
| testValue := "1" | ||
|
|
||
| cmdFlags.Set("serviceAccount", testValue) | ||
| if vString, err := cmdFlags.GetString("serviceAccount"); err == nil { | ||
| testDecodeJson_Config(t, fmt.Sprintf("%v", vString), &actual.ServiceAccount) | ||
|
|
||
| } else { | ||
| assert.FailNow(t, err.Error()) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Consider consolidating the test cases for kubeRayCrdVersion and serviceAccount with other similar string flag tests to reduce code duplication. The test structure and validation logic is identical across multiple test cases. A similar issue was also found in flyteplugins/go/tasks/plugins/k8s/spark/config_flags_test.go (line 270-283).
Code suggestion
Check the AI-generated fix before applying
@@ -97,159 +97,25 @@ func TestConfig_SetFlags(t *testing.T) {
actual := Config{}
cmdFlags := actual.GetPFlagSet("")
assert.True(t, cmdFlags.HasFlags())
- // ... existing test cases ...
+ testStringFlag := func(t *testing.T, flagName string, field *string) {
+ t.Run(fmt.Sprintf("Test_%s", flagName), func(t *testing.T) {
+ t.Run("Override", func(t *testing.T) {
+ testValue := "1"
+ cmdFlags.Set(flagName, testValue)
+ if vString, err := cmdFlags.GetString(flagName); err == nil {
+ testDecodeJson_Config(t, fmt.Sprintf("%v", vString), field)
+ } else {
+ assert.FailNow(t, err.Error())
+ }
+ })
+ })
+ }
+ testStringFlag(t, "kubeRayCrdVersion", &actual.KubeRayCrdVersion)
+ testStringFlag(t, "serviceAccount", &actual.ServiceAccount)
}
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| // The generic dynamic log link is special. We're going to use the string `Generic` to signal that we're using it. | ||
| // TODO: decide if it's worth doing this in a different way. | ||
| if cfg.GenericDynamicLogLinksEnabled { | ||
| dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []string{""}, MessageFormat: core.TaskLog_JSON}) |
There was a problem hiding this comment.
Consider using []tasklog.TemplateURI instead of []string for DynamicTemplateURIs to maintain type consistency. The TemplateLogPlugin struct expects DynamicTemplateURIs to be of type []TemplateURI.
Code suggestion
Check the AI-generated fix before applying
| dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []string{""}, MessageFormat: core.TaskLog_JSON}) | |
| dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []tasklog.TemplateURI{""}, MessageFormat: core.TaskLog_JSON}) |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| func (_m *TaskOverrides) OnGetExtendedResources() *TaskOverrides_GetExtendedResources { | ||
| c_call := _m.On("GetExtendedResources") | ||
| return &TaskOverrides_GetExtendedResources{Call: c_call} |
There was a problem hiding this comment.
Consider if changing OnGetExtendedResources() to OnGetContainerImage() is intentional as it appears to be replacing the extended resources mock setup with container image mock setup.
Code suggestion
Check the AI-generated fix before applying
| func (_m *TaskOverrides) OnGetExtendedResources() *TaskOverrides_GetExtendedResources { | |
| c_call := _m.On("GetExtendedResources") | |
| return &TaskOverrides_GetExtendedResources{Call: c_call} | |
| func (_m *TaskOverrides) OnGetContainerImage() *TaskOverrides_GetContainerImage { | |
| c_call := _m.On("GetContainerImage") | |
| return &TaskOverrides_GetContainerImage{Call: c_call} | |
| } | |
| func (_m *TaskOverrides) OnGetContainerImage() *TaskOverrides_GetContainerImage { | |
| c_call := _m.On("GetContainerImage") | |
| return &TaskOverrides_GetContainerImage{Call: c_call} |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| r0 = rf() | ||
| } else { | ||
| if ret.Get(0) != nil { | ||
| r0 = ret.Get(0).(string) | ||
| r0 = ret.Get(0).(*flyteidlcore.ExtendedResources) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider validating the return value from ret.Get(0) before type assertion to avoid potential panic if the value is nil.
Code suggestion
Check the AI-generated fix before applying
| } | |
| if v := ret.Get(0); v != nil { | |
| r0 = v.(string) | |
| } else { | |
| r0 = "" | |
| } |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| return &TaskOverrides_GetExtendedResources{Call: c_call} | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider keeping both OnGetContainerImage and OnGetExtendedResources methods since removing the container image method could break existing tests that depend on it.
Code suggestion
Check the AI-generated fix before applying
| return &TaskOverrides_GetExtendedResources{Call: c_call} | |
| } | |
| func (_m *TaskOverrides) OnGetExtendedResources() *TaskOverrides_GetExtendedResources { | |
| c_call := _m.On("GetExtendedResources") | |
| return &TaskOverrides_GetExtendedResources{Call: c_call} | |
| } | |
| func (_m *TaskOverrides) OnGetContainerImage() *TaskOverrides_GetContainerImage { | |
| c_call := _m.On("GetContainerImage") | |
| return &TaskOverrides_GetContainerImage{Call: c_call} |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -206,21 +235,44 @@ | |||
| }) | |||
| } | |||
|
|
|||
| genericDynamicLogLinks := taskTemplateHasGenericDynamicLogLinksEnabled(input.TaskTemplate) | |||
There was a problem hiding this comment.
Consider checking if input.TaskTemplate is nil before calling taskTemplateHasGenericDynamicLogLinksEnabled() to avoid potential nil pointer dereference.
Code suggestion
Check the AI-generated fix before applying
| genericDynamicLogLinks := taskTemplateHasGenericDynamicLogLinksEnabled(input.TaskTemplate) | |
| var genericDynamicLogLinks []genericDynamicLogLink | |
| if input.TaskTemplate != nil { | |
| genericDynamicLogLinks = taskTemplateHasGenericDynamicLogLinksEnabled(input.TaskTemplate) | |
| } |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| } else if p.Name == dynamicLogLinkType { | ||
| if p.Name == dynamicLogLinkType { | ||
| for _, match := range taskConfigVarRegex.FindAllStringSubmatch(dynamicTemplateURI, -1) { |
There was a problem hiding this comment.
Consider removing the redundant nested if condition since it's checking the same condition as the outer if statement
Code suggestion
Check the AI-generated fix before applying
| } else if p.Name == dynamicLogLinkType { | |
| if p.Name == dynamicLogLinkType { | |
| for _, match := range taskConfigVarRegex.FindAllStringSubmatch(dynamicTemplateURI, -1) { | |
| } else if p.Name == dynamicLogLinkType { | |
| for _, match := range taskConfigVarRegex.FindAllStringSubmatch(dynamicTemplateURI, -1) { | |
| for _, match := range taskConfigVarRegex.FindAllStringSubmatch(dynamicTemplateURI, -1) { |
Code Review Run #51871a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
Cleaning stale PRs. Please reopen if you wan to discuss this further. |
Tracking issue
#4773
Why are the changes needed?
We need support for dynamic log links defined by tasks.
What changes were proposed in this pull request?
Add a flag to the propeller config called
GenericDynamicLogLinksEnabled, which is self-explanatory. We then use a dynamic log link guard called "Generic" to guide the addition of generic dynamic log links (what I'm calling these log links defined in tasks - a better name could be in order).How was this patch tested?
Unit tests and local sandbox.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR implements generic dynamic log links support in Flyte, introducing a new 'GenericDynamicLogLinksEnabled' flag and infrastructure for task-defined dynamic log links. The implementation includes template-based log link processing and configuration updates across k8s array, spark, and ray plugins. The changes enable flexible log management through customizable log link templates.Unit tests added: True
Estimated effort to review (1-5, lower is better): 4