feat: Add mandatory-module label to mandatory ModuleTemplate #2033
Conversation
nesmabadr
added
the
do-not-merge/work-in-progress
kyma-bot
added
cla: yes
kyma-bot
added
size/L
nesmabadr
force-pushed
the
add_label_to_mandatory_module
branch
from
48069a4
to
3a137dc
Compare
nesmabadr
force-pushed
the
add_label_to_mandatory_module
branch
from
3a137dc
to
fc75660
Compare
|
/cla |
|
Successfully reached out to cla-assistant.io to initialize recheck of PR #2033 |
There was a problem hiding this comment.
Maybe I missed the discussion while I was on vacation, but I am honestly not sure what we gain from this. The lookup for mandatory modules is maybe slightly simpler as we filter directly by label, but we still need the for loop to map the list to a map. So not much of a simplification or performance gain.
But this comes at the price of rather complex handling in the kyma controller to ensure this label. Maybe I missing the obvious reason why we want to use introduce this label?
| func MandatoryModuleTemplateHasExpectedLabel(ctx context.Context, clnt client.Client, moduleName, key, value string, | ||
| ) error { | ||
| mandatoryModuleTemplates, err := templatelookup.GetMandatory(ctx, clnt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var moduleTemplate *v1beta2.ModuleTemplate | ||
| for _, moduleTemplateInfo := range mandatoryModuleTemplates { | ||
| if moduleTemplateInfo.ModuleTemplate.Labels[shared.ModuleName] == moduleName { | ||
| moduleTemplate = moduleTemplateInfo.ModuleTemplate | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if moduleTemplate == nil { | ||
| return fmt.Errorf("module template not found, %s", moduleName) | ||
| } | ||
|
|
||
| if moduleTemplate.Labels[key] != value { | ||
| return fmt.Errorf("label %s:%s not found", key, value) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I am okay with this as is, but one idea to simplify may be to use the lookup by label already. I.e., get all ModuleTemplates with operator.kyma-project.io/mandatory-module: true, assert that the len of returned ModuleTemplates is 1 and the entry has the expected module name.
| Labels: map[string]string{ | ||
| shared.IsMandatoryModule: shared.EnableLabelValue, | ||
| }, |
There was a problem hiding this comment.
I am a bit confused by this change. Isn't this applying the mandatory module label to all tests?
| mandatoryModuleTemplateList := &v1beta2.ModuleTemplateList{} | ||
| labelSelector := k8slabels.SelectorFromSet(k8slabels.Set{shared.IsMandatoryModule: shared.EnableLabelValue}) | ||
| if err := kymaClient.List(ctx, mandatoryModuleTemplateList, | ||
| &client.ListOptions{LabelSelector: labelSelector}); err != nil { | ||
| return nil, fmt.Errorf("could not list mandatory ModuleTemplates: %w", err) |
There was a problem hiding this comment.
Maybe we could also add a field selector client.MatchingFields{"metadata.deletionTimestamp": ""}. Then we could get rid of filtering in the for loop bellow.
Description
Changes proposed in this pull request:
Related issue(s)
Resolves #1198