-
Notifications
You must be signed in to change notification settings - Fork 187
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
New Runtime #287
New Runtime #287
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
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.
Left some minor comments for improvement. But essentially everything you did works great. 😍
Biggest question and chance for improvement I see is: would be even more awesome to spawn the extensions with pman, but still like this: ocis <extension>
. Right now this starts the extension without using pman. Goal should be to have all extensions spawned and managable through pman, for both ocis server
and ocis <extension>
. Problem is, that the way pman spawns the process would cause a recursion... ideas?
pkg/micro/runtime/runtime.go
Outdated
args := []string{os.Args[0]} | ||
all := append(Extensions, MicroServices...) | ||
for i := range all { | ||
arg0 := process.NewProcEntry( |
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.
Could be renamed to args
to be in line with the callee's signature.
pkg/micro/runtime/runtime.go
Outdated
all[i], | ||
[]string{all[i]}..., | ||
) | ||
var arg1 int |
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.
Could be renamed to reply
to be in line with the callee's signature.
pkg/command/server_simple.go
Outdated
runtime.Start() | ||
runtime.Trap() | ||
} | ||
runtime.Start() |
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.
Return the (possible) error from here, instead of nil two lines below.
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.
server_simple
seems to be broken... but looks like it already was before.
Oh and of course: changelog. ;-D |
the process manager will be included into the ocis toolchain indeed, resulting in, i.e when running |
Thanks! I already rebased a while back, but encounter a race condition on reva. It finds out that Solved it for the new runtime on a hacky way, will have to tackle it on reva upstream. |
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.
🐙 ❤️
This has some implications. Since micro/micro 2.4.0 with the addition of an There is however a // Inspect a token
func (n *noop) Inspect(token string) (*Account, error) {
return &Account{ID: uuid.New().String(), Issuer: n.Options().Namespace}, nil
} Where Issuer, for whatever reason, is empty. By the time this field is checked, I submitted a PR upstream that, if merged, would allow to, again, call the micro |
cc @butonic , I believe this impacts the way you're currently testing the accounts queries through the |
[tests-only] skip tests that are tagged @skipOnOcis-OC-Storage
The runtime itself lives here, and comments on its design can, and should, be appended to this pull request.
Rationale: micro/go-micro is based on running from sources, and we do not want that for the time being. We should also be in control of
How We Run What We Run™
, and therefore this approach fits better than using a third party process manager.