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.

Sign up for GitHub

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

Jump to bottom

support exposure analysis + tests #8

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

support exposure analysis + tests #8

wants to merge 15 commits into from

Conversation

shireenf-ibm
Copy link
Owner

@shireenf-ibm shireenf-ibm commented Aug 11, 2024 •
edited
Loading

Description

  • supporting exposure analysis for connectivity map
  • adding unit test + bats tests
    - netpol-analyzer is imported locally in go.mod

@shireenf-ibm shireenf-ibm marked this pull request as draft August 11, 2024 16:57
@adisos
Copy link
Collaborator

adisos commented Aug 12, 2024

should also update documentation at https://github.com/shireenf-ibm/stackrox/tree/master/roxctl/netpol/connectivity/map

adisos
adisos reviewed Aug 12, 2024
View reviewed changes
roxctl/netpol/connectivity/map/map.go Outdated
defaultOutputFileNamePrefix = "connlist."
defaultOutputFormat = "txt"
defaultOutputFileNamePrefix = "connlist."
exposureOutputFileNamePrefix = "exposure."
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this required? it's the same connlist command, so I would not change the name of the default file name based on some arg...

Copy link
Owner Author

Choose a reason for hiding this comment

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

i see.
i did this for case we want to run on same dir twice (with and without the exposure flag, thought maybe not to "erase" previous results), however a user may choose to provide his output file name to avoid this

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

@adisos
Copy link
Collaborator

adisos commented Aug 12, 2024

Missing also tests with dot output format

@shireenf-ibm
Copy link
Owner Author

hi @adisos
in last commit, i have added two binaries files for mac (for darwin_amd64 and darwin_arm64); so you can pull and forward them if needed
(this commit will be reverted later)

@adisos
Copy link
Collaborator

adisos commented Sep 9, 2024

  • extend documentation (a section with graph example, more explanation about the feature and the output in text and dot formats).
  • remove some redundant tests

@shireenf-ibm shireenf-ibm force-pushed the connectivity_map_with_exposure_analysis branch from 3c8dd94 to 51f289c Compare September 12, 2024 10:14
@adisos
Copy link
Collaborator

adisos commented Sep 25, 2024

looks good! let's update this branch with most recent main of stackrox

adisos
adisos reviewed Sep 25, 2024
View reviewed changes
roxctl/netpol/connectivity/map/map.go Outdated
defaultOutputFileNamePrefix = "connlist."
defaultOutputFormat = "txt"
defaultOutputFileNamePrefix = "connlist."
exposureOutputFileNamePrefix = "exposure."
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this prefix exposure. be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

had a comment about this previously, (marked resolved) now marked as unresolved..

Copy link
Owner Author

Choose a reason for hiding this comment

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

yes it should, the rebase was not successful (some commits were not picked)
i am working on fixing this

@shireenf-ibm shireenf-ibm force-pushed the connectivity_map_with_exposure_analysis branch from 6741a1c to 21bffdb Compare September 25, 2024 11:57
@shireenf-ibm shireenf-ibm force-pushed the connectivity_map_with_exposure_analysis branch from e42ee5a to 3ca4bd9 Compare September 25, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisos adisos adisos left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shireenf-ibm @adisos