introducing systemservices #535
Conversation
ilgooz
force-pushed
the
feature/system-services
branch
from
1e72b86
to
22bee39
Compare
ilgooz
force-pushed
the
feature/system-services
branch
from
1a5ed69
to
8c6555b
Compare
systemservices/resolver/resolver.go
Outdated
| case "notFound": | ||
| return "", fmt.Errorf("address for service id %s not found", serviceID) | ||
| case "error": | ||
| return "", errors.New(e.OutputData["error"].(string)) |
There was a problem hiding this comment.
What about creating one error type that can contains the error message but also the outputKey?
So we don't have a 1000s different error type but we can access the outputKey returned by the task.
There was a problem hiding this comment.
For this one, I don't quite understand what do you mean? The line you highlighted is about error output key and it's only returned from the service when there is an internal error. Output key is always the same but error message can be different depending on the internal error happened in the service.
But I think we need a custom error type for notFound output and here is the implementation.
|
|
||
| switch e.Output { | ||
| case "found": | ||
| return e.OutputData["address"].(string), nil |
There was a problem hiding this comment.
Just as a note for possible more complicated type conversion that are done one multiple variable: We can create structs and json.Unmarshal.
There was a problem hiding this comment.
I chose to do type conversion and added some notes about that. We should improve the api package to accept any types otherwise using json.Unmarshal for serialization can decrease the performance that can be prevented by having a better api.
* x/xos: add DirExists()
…rvices.go, improve docs
* core: add temporary lines to manually test systemservices pkg
There was a problem hiding this comment.
We still can't use it in api package, we need to resolve this issue first (https://forum.mesg.com/t/the-import-cycle-between-api-systemservices-packages/105/7)
|
@krhubert I think we can solve that issue within another PR and we don't have a decision on that one yet. I need this PR to be merged for my upcoming PRs. |
It's hard to use -source in command line
|
https://forum.mesg.com/t/the-import-cycle-between-api-systemservices-packages/105/9 needs to be resolved before merging this PR |
|
@mesg-foundation/core please review |
|
manual tests are ok |
|
I played with it and it's great, i was able to do some monitoring of the core with the influxdb service really easily so for me it's good to go. Few feedbacks:
I didn't rename the |
|
@antho1404 I fixed the some missing -source to source path changes with my last commit, it's working now. I've manually tested everything with the resolver service on my machine. But I personally like to have some kind of prefix for sources like -sources because otherwise it'll look like just an another system service wrapper if we don't explicitly group wrappers under a folder like wrappers. -source to source change breaks my next PRs as well as the change on APIs of the systemservices/* but it's ok, I can do the necessary updates while rebasing next PR's to this one. |
No description provided.