Skip to content
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

exposure analysis test #316

Conversation

shireenf-ibm
Copy link
Contributor

issue #236

sub task :

    • basic tests for exposure analysis results

@shireenf-ibm shireenf-ibm marked this pull request as draft March 4, 2024 18:13
@shireenf-ibm shireenf-ibm requested a review from adisos March 4, 2024 18:14
@shireenf-ibm shireenf-ibm mentioned this pull request Mar 4, 2024
@shireenf-ibm shireenf-ibm requested review from adisos and removed request for adisos March 11, 2024 17:01
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, few more comments

pkg/netpol/connlist/exposure_analysis_test.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_analysis_test.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_analysis_test.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_analysis_test.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_analysis_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it confusing that the new tests dir for exposure analysis is placed within the the dir of other tests

shireenf-ibm and others added 5 commits March 19, 2024 12:24
Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>
Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>
Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>
Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>
Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another question: do we handle containment of labels for representative peers?
for example, a potential connection to ns with label {A: val1} and with conn: all , is including also a potential connection to ns with labels {A: val1, B: val2} , for all contained connections. (which makes such a potential connection redundant).

If not, this can be considered in a separate issue.

@shireenf-ibm
Copy link
Contributor Author

another question: do we handle containment of labels for representative peers? for example, a potential connection to ns with label {A: val1} and with conn: all , is including also a potential connection to ns with labels {A: val1, B: val2} , for all contained connections. (which makes such a potential connection redundant).

If not, this can be considered in a separate issue.

it is not handled (added a new task )

@shireenf-ibm
Copy link
Contributor Author

shireenf-ibm commented Mar 19, 2024

I find it confusing that the new tests dir for exposure analysis is placed within the the dir of other tests

What do you suggest?

the thing is that the tests dir is our global testing dir (those tests will be also used in the connlist_analyzer_test too for output testing; i.e will be added to the good path tests with the new flag of exposure analysis); so are assumed to be under tests.
I grouped them under a tests/exposure_analysis_tests so I can easily remember and find the new tests

pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested review from adisos and removed request for adisos April 1, 2024 08:53
pkg/netpol/connlist/exposure_analysis.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_analysis.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested a review from adisos April 1, 2024 14:44
pkg/netpol/connlist/exposure_analysis.go Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/exposure_map.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested a review from adisos April 2, 2024 06:55
@shireenf-ibm shireenf-ibm requested review from adisos and removed request for adisos April 2, 2024 11:48
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shireenf-ibm shireenf-ibm marked this pull request as ready for review April 2, 2024 12:29
@shireenf-ibm shireenf-ibm merged commit 6a702b5 into new_exposure_analysis_first_branch Apr 2, 2024
2 checks passed
@shireenf-ibm shireenf-ibm deleted the test_exposure_analysis_behavior branch April 2, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants