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

allows to use multiple ownership transfer methods for own op #279

Merged
merged 3 commits into from
Aug 25, 2022

Conversation

jkralik
Copy link
Member

@jkralik jkralik commented Aug 24, 2022

No description provided.

@jkralik jkralik requested a review from Danielius1922 August 24, 2022 10:06
@jkralik jkralik force-pushed the jkralik/feature/own-array-otm-clients branch from c40cc02 to b64c1bc Compare August 24, 2022 10:07
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022

Codecov Report

Merging #279 (5e309f0) into main (be75542) will increase coverage by 0.32%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
+ Coverage   60.23%   60.56%   +0.32%     
==========================================
  Files          64       64              
  Lines        4059     4067       +8     
==========================================
+ Hits         2445     2463      +18     
+ Misses       1266     1255      -11     
- Partials      348      349       +1     
Impacted Files Coverage Δ
client/client.go 62.93% <ø> (ø)
client/core/getResource.go 31.81% <ø> (ø)
client/deviceOwnershipBackend.go 0.00% <0.00%> (ø)
client/deviceOwnershipNone.go 0.00% <0.00%> (ø)
client/refDevice.go 91.48% <ø> (ø)
client/deviceOwnershipSDK.go 54.76% <70.00%> (+0.91%) ⬆️
client/options.go 45.03% <80.00%> (-1.06%) ⬇️
client/core/ownDevice.go 57.48% <100.00%> (+1.52%) ⬆️
client/ownDevice.go 40.90% <100.00%> (ø)
client/core/client.go 83.56% <0.00%> (-4.11%) ⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jkralik jkralik force-pushed the jkralik/feature/own-array-otm-clients branch from b64c1bc to 7aaf3e0 Compare August 24, 2022 10:42
@jkralik jkralik marked this pull request as ready for review August 24, 2022 10:42
@jkralik jkralik force-pushed the jkralik/feature/own-array-otm-clients branch from 7aaf3e0 to e9c10b5 Compare August 24, 2022 10:56
@jkralik jkralik force-pushed the jkralik/feature/own-array-otm-clients branch from e9c10b5 to ab0226f Compare August 24, 2022 12:03
}
if !supportOtm {
return MakeUnavailable(fmt.Errorf("ownership transfer method '%v' is unsupported, supported are: %v", otmClient.Type(), ownership.SupportedOwnerTransferMethods))
otmClient := findOTMClient(otmClients, ownership.SupportedOwnerTransferMethods)
Copy link
Member

Choose a reason for hiding this comment

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

This gives you the first matching client, but you can have multiple matches. Is taking the first match always correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fine. It is not expected that clients add there multiple otm with the same type.

Copy link
Member

@Danielius1922 Danielius1922 Aug 25, 2022

Choose a reason for hiding this comment

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

Both otmClients and deviceSupportedOwnerTransferMethods are arrays though, so even if you don't have multiple clients of the same type you can have multiple matches.

Lets say that:

otmClients = []{client for ManufacturerCertificate, client for Self}
deviceSupportedOwnerTransferMethods = []{ManufacturerCertificate, Self}

findOTMClient will always return the first match (client for ManufacturerCertificate in this case). I don't know what's the expected behavior of the Own function, so this might or might not be a problem. But the function is dependent on the order of the values in the otmClients slice, which I think we should at least document.
Also isn't possible that in this example that owning with ManufacturerCertificate client fails, but owning with Self would succeed? However, the Self client won't be tried and the user of the function doesn't know which client was used and failed. I guess a custom Error type which would have a type of the client used would be best here, so the caller could remove it from the otmClients slice and retry.

@jkralik jkralik merged commit 325917d into main Aug 25, 2022
@jkralik jkralik deleted the jkralik/feature/own-array-otm-clients branch August 25, 2022 09:42
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