-
Notifications
You must be signed in to change notification settings - Fork 5
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
[SDK-443] Add Sources & Verifiers #23
Conversation
…exported, change image type to be string
In the last commit I changed the anchors, sources & verifiers fields to be unexported so that we can restrict access to them through accessor methods. This has the disadvantage of a slight bit of duplication in each of the attribute classes, but Mike and I had a chat and believe this is better than having the potential for unwanted modifications of these three slices. |
…sent, add scenarioID
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.
Good work, well done.
Couple of comments but I don't see any need to hold this up further now.
|
||
// GetVerifiers returns the anchors which identify how and when an attribute value was verified by another provider. | ||
func GetVerifiers(anchors []*Anchor) (sources []*Anchor) { | ||
return filterAnchors(anchors, AnchorTypeVerifier) |
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.
Since we already have Type as a string on each Anchor struct (as taken from the certificate), can't we just filter on that? What's the additional type enum for?
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.
Update, I'm a bit wrong here, we've just talked this through....
The anchor type comes to us as an 'extensionOId' e.g. '1.3.6.1.4.1.47127.1.1.1'. We are not given a nice, neat String. The String's are introduced by the SDK to make the OId more meaningful.
The apprach here matches the Java SDK - map the OId to an Enum as a convenient way to convert that OId into a nice string. If we ever get an OId we don't know about, we map it to unknown.
We might to want to improve on this so I've created SDK-683, but for now the work done here is good to go.
…function, tidy up tests
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.
Well done Ed, it looks much better!
Just few Questions.
|
||
func filterAnchors(anchors []*Anchor, anchorType Type) (result []*Anchor) { | ||
for _, v := range anchors { | ||
if v.Type == anchorType { |
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.
Does the Anchor Object include the AnchorType?
If so what's the value of the type e.g 'source' or the old e.g '1.3.6.1.4.1.47127.1.1.1'?
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.
Discussed in https://lampkicking.atlassian.net/browse/SDK-683: having enum type is fine for the foreseeable future, and we won't expose OID
Value interface{} | ||
Anchors []*anchor.Anchor | ||
value interface{} | ||
anchors []*anchor.Anchor |
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.
Does this hold a list of Anchors e.g [AnchorObj1, AnchorObj2, ...] or a Map of Anchors as ['1.3.6.1.4.1.47127.1.1.1' => AnchorObj, ...]?
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 believe it holds a list as this is a slice. I don't think Go has associative arrays.
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.
Yes this is a slice, a bit like a list - there's basically nothing a list can do that a slice can't (in Go), but lists have the disadvantage of not being strongly typed. You can have a map, but I don't think that is necessary here
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.
Nice one Ed!
Value interface{} | ||
Anchors []*anchor.Anchor | ||
value interface{} | ||
anchors []*anchor.Anchor |
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 believe it holds a list as this is a slice. I don't think Go has associative arrays.
I found a couple of issues with the method descriptions, so I will review all of these before releasing.