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

Added ComMatrix method; Changed private func to public #17

Closed
wants to merge 3 commits into from

Conversation

shirmoran
Copy link
Collaborator

@shirmoran shirmoran commented Jul 16, 2024

This PR changed functions in order to use them in a new openshift\origin test specified here :
Changed "addFromFile" function (in types.go) to a public function to allow using it in the mentioned above test.

@aabughosh aabughosh requested a review from sabinaaledort July 16, 2024 09:04
types/types.go Outdated
@@ -160,3 +160,36 @@ func (m ComMatrix) Contains(cd ComDetails) bool {

return false
}

// Returns True if both ComMatrixes have exactly the same ComMatrix details.
func (m ComMatrix) Equals(other ComMatrix) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not to use the Diff function?
u need from diff to be equal to nothing from 2 sides

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sabinaaledort also can use that on the test of the origin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we don't need the Diff matrices is this case, it occupies unnecessary space.

@sabinaaledort
Copy link
Collaborator

I'm not sure the right thing to do is to keep the equals func in the commatrix repo instead of just use in the test.
It's an unused func in the library and makes an extra dependence between the test and the lib, for example if we want to ignore some ports in the test diff we will have to make a change in the matrix repo as well

@sabinaaledort
Copy link
Collaborator

I think using addFromFile func for the test is an interesting idea but I think you should consider to compare the csv files directly, it's an an extra dependence that might cause bugs. For example if we have a bug in the addFromFile func and we miss a port the test will fail...

@shirmoran shirmoran closed this Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants