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

Add routines to compare measurements against reference #167

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jraman567
Copy link
Contributor

Adds a compare routine to some of the measurement types.

@jraman567
Copy link
Contributor Author

I can't figure out the lint error; I'm unsure how to fix it.

@thomas-fossati
Copy link
Contributor

I can't figure out the lint error; I'm unsure how to fix it.

I suggest giving a go at:

goimports -local "github.com/veraison/corim" -w .

@jraman567
Copy link
Contributor Author

I suggest giving a go at:

goimports -local "github.com/veraison/corim" -w .

Thanks, @thomas-fossati; that helped to fix it. As Dionna suggested in the meeting, the fix needed a new line between "fmt" and the following line.

I'm working on the test coverage and will post a new revision of the changes shortly. Thank you!

Copy link
Contributor

@setrofim setrofim left a comment

Choose a reason for hiding this comment

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

As a general note, it's weird for Compare() method to raise an error. Two values not being equal is not automatically considered an error condition. Also, I would expect a method called "Compare" to do a relative comparison, rather than equality test.

I would rename the method to Equal and have it return a bool instead.

(Alternatively, you could keep it as "Compare" and have it return an int, with 1 indicating o > r, -1 indicating r > o, and 0 indicating o == r; however, that would require chainging the implementations as well, and would require for all types in question to have partial ordering, which I don't think we want.)

comid/digests.go Outdated Show resolved Hide resolved
comid/digests.go Outdated Show resolved Hide resolved
comid/flagsmap.go Outdated Show resolved Hide resolved
comid/integrityregisters.go Outdated Show resolved Hide resolved
comid/macaddr.go Outdated Show resolved Hide resolved
@thomas-fossati
Copy link
Contributor

Thanks, @thomas-fossati; that helped to fix it. As Dionna suggested in the meeting, the fix needed a new line between "fmt" and the following line.

Do you use vim or VS Code? If so, the popular go plugins (e.g., vim-go) should automatically update the import statements order transparently.

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

I agree with @setrofim high-level comment.

I think we should have:

  • a Compare method for types for which ordering makes sense, e.g., SVNs -- and it should return -1,0,1 rather than error;
  • an Equal method for types for which we are only concerned about strict equality (e.g., digests) -- and this could return a bool.

@setrofim
Copy link
Contributor

setrofim commented Feb 12, 2025

an Equal method for types for which we are only concerned about strict equality (e.g., digests)

I think we should have Equal for the other types as well (even if it's just implemented as o.Compare(r) == 0), since that would allow you to handle of values uninformly as interface{ Equal() bool } in situatations where you only care about exact values.

@thomas-fossati
Copy link
Contributor

an Equal method for types for which we are only concerned about strict equality (e.g., digests)

I think we should have Equal for the other types as well (even if it's just implemented as o.Compare(r) == 0), since that would allow you to handle of values uninformly as interface{ Equal() bool } in situatations where you only care about exact values.

WFM

@jraman567
Copy link
Contributor Author

As a general note, it's weird for Compare() method to raise an error. Two values not being equal is not automatically considered an error condition. Also, I would expect a method called "Compare" to do a relative comparison, rather than equality test.

I also prefer a boolean return value. If we cannot perform the comparison due to an error, should we log it and return false for a result?

I would rename the method to Equal and have it return a bool instead.

It's not always equality; the type governs the semantics of comparison.

In the SVN type, for example, the comparison should be equality for the ExactValueType and greater-than-or-equal for MinValueType.

@jraman567
Copy link
Contributor Author

OK, I'll push a PR with recommended changes. Thanks @thomas-fossati & @setrofim for the review.

- refactor Version type into a separate file
- add a method to test if a Version type equals a reference

Signed-off-by: Jagannathan Raman <[email protected]>
@jraman567 jraman567 force-pushed the measurement-compare branch 2 times, most recently from 1695f74 to 4b374fc Compare February 12, 2025 16:58
comid/svn.go Outdated Show resolved Hide resolved
comid/svn.go Show resolved Hide resolved
Add methods to test SVN against a reference for equality. Also
add a Compare method for TaggedMinSVN

Signed-off-by: Jagannathan Raman <[email protected]>
Add a method to test if Digests are equal to reference values

Signed-off-by: Jagannathan Raman <[email protected]>
Add a method to test if a FlagsMap type equals a reference

Signed-off-by: Jagannathan Raman <[email protected]>
Add a method to test if a RawValue type equals a reference

Signed-off-by: Jagannathan Raman <[email protected]>
Add a method to test if a MACaddr type equals a reference

Signed-off-by: Jagannathan Raman <[email protected]>
Add a routine to compare IntegrityRegisters against a reference

Signed-off-by: Jagannathan Raman <[email protected]>
@jraman567
Copy link
Contributor Author

@thomas-fossati @setrofim could you please confirm if the PR looks good? Thanks!

comid/svn.go Show resolved Hide resolved
comid/svn.go Show resolved Hide resolved
Copy link
Contributor

@thomas-fossati thomas-fossati 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, thanks @jraman567 !

There's one point to discuss about what to do with RawValue.

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