-
Notifications
You must be signed in to change notification settings - Fork 26
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
Intentions module input configuration #987
base: main
Are you sure you want to change the base?
Conversation
d45e5f1
to
9eec5ed
Compare
Did a rebase, this change does not require release engineers to review anymore. Removing you all as reviewers thanks. |
This PR looks good, but not marking it as approved yet. We will utilize this as the feature branch for all intentions work, so there will be other PRs merged into this. |
return ": (*IntentionsServicesConfig)(nil)" | ||
} | ||
|
||
if len(c.Names) > 0 { |
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.
I am not sure about the correctness of this logic
I understand that we want to specify either Names OR Regex, but that is business logic, and the struct doesn't know about this restriction. Therefore, IF I create an IntentionsServicesConfig object with both Names and Regex defined, I expect the GoString()
method to return a string with both fields, not just Names.
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.
This logic behind this is for the expected use case that both fields wouldn't be defined. If you create IntentionsServicesConfig object with both field defined, the validate function that is prior to this would fail. I could see putting an error message here instead if both are configured but showing both didn't make sense to me because the output would be :
&IntentionsModuleInputConfig{&IntentionsMonitorConfig{Datacenter:dc, Namespace:namespace, Source Services: Names: Regexp:^web.*, Destination Services: Names: Regexp:^api.*}}
Could you explain more what the use case for showing both names and regexp would be? I think it would steer people into the wrong direction that you could use both and lead to test failure.
Edited test name to be correct and more reflective of what it does : "regexp_same"
Added additional test case for "happy_path_different_type" and added back previous test case. This additional test case, test that two different paths (on being Intentions module input) merge.
Changed test name to "regexp_same"
Readability change Co-authored-by: AJ Jwair <[email protected]>
Readability change Co-authored-by: AJ Jwair <[email protected]>
Readability change Co-authored-by: AJ Jwair <[email protected]>
Readability change Co-authored-by: AJ Jwair <[email protected]>
Change Sprintf to be one line in GoString
Change Sprintf to be one line in GoString
2a6495d
to
a733ece
Compare
Change: added new configuration for
module_input
"intentions"
and updated necessary module input dependenciesCommits :
source_services
anddestination_services
used in the"intentions"
blockmodule_input
"intentions"
Sample
"intentions" module_input