feat(#10): collecting and sending failed links back to cloud provider via server #20
Conversation
… provider via server)
fixing go.mod
There was a problem hiding this comment.
I think in terms of the logic here for this provider, it looks great and it's good to actually return that error back up to the provider SDK itself. #10 is probably going to require a change in the actual wasmCloud host to do something with the failed link, so I think it would be best tracked in the host itself 😄
Because of that, I think this issue addresses #10 in that it shows the best way to handle links and link errors for the time being. Just requesting changes around some potential refactors
…different for a different project
There was a problem hiding this comment.
I've got one more question about how we keep track of links in the provider SDK itself, and it looks like you may also need to sign the commits in this PR (the Details button next to the Failed DCO action will show you the git command to do so) but this looks really nice 👍🏻
| if l.SourceID == wp.Id { | ||
| err := wp.putSourceLinkFunc(l) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| wp.sourceLinks[l.Target] = l | ||
| if l.SourceID == wp.Id { | ||
| return wp.putSourceLinkFunc(l) | ||
| } else if l.Target == wp.Id { | ||
| err := wp.putTargetLinkFunc(l) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| wp.targetLinks[l.SourceID] = l |
There was a problem hiding this comment.
Could you comment on why you removed this specific logic? With our example provider we do keep track of links explicitly, however I think in the general case providers shouldn't have to explicitly keep track of links and should be able to store whatever data necessary. Does that make sense?
Storing the links here is also necessary for the checks in the provider SDK to prevent duplicate links
There was a problem hiding this comment.
I think I may not be fully understanding the business logic here, or the request to refactor the link logic outside of the provider. This logic, if I'm understanding it correctly, is further down the chain and being stored as part of these functions and bubbling the error back up to the provider:
func (p *Provider) handleNewSourceLink(link provider.InterfaceLinkDefinition) error {
log.Println("Handling new source link", link)
err := p.establishSourceLink(link)
if err != nil {
log.Println("Failed to establish source link", link, err)
p.failedSourceLinks[link.Target] = link
return err
}
p.sourceLinks[link.Target] = link
return nil
}
func (p *Provider) handleNewTargetLink(link provider.InterfaceLinkDefinition) error {
log.Println("Handling new target link", link)
err := p.establishTargetLink(link)
if err != nil {
log.Println("Failed to establish target link", link, err)
p.failedTargetLinks[link.SourceID] = link
return err
}
p.targetLinks[link.SourceID] = link
return nil
}
There was a problem hiding this comment.
Ah to clarify, I like that the provider SDK itself is keeping track of the links in order to put/delete them from a map and that the developer writing a provider handler doesn't need to keep that map established (even though we do in our example)
|
Hey @tom-fitz just coming back to this, apologies for the delay between reviews. I think part of the confusion here is between the example that we have in this repository and what the provider SDK handles automatically, and just as a general principle I'm trying to give the user of the SDK as much information as possible while keeping track of bookkeeping behind the scenes. In the link example, I want the provider SDK to do the tracking, the duplication checking, and the deletion of links from the map, but we want to make sure to deliver the link to the callback given to the SDK so that the user can do what they want with it. Let me know if that makes sense / if you have questions |
Feature or Problem
Identify which links did not pass validation and pass them back to the wasm cloud provider.
Related Issues
Possibly fixes #10 depending on what type of information you want passed to the cloud provider. Is it enough to just identify which links failed, or should we also include a message on what failed? I left the code open ended so that we could discuss based on the needs of the cloud provider and add validation where we see fit.
Tech Debt
I also corrected some local errors that were causing this code not to compile.
slogis behind and does not work on1.22only1.20. Also Golang does not have apatchversion, onlymajor.minor