-
Notifications
You must be signed in to change notification settings - Fork 990
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
changes to support specific run service URL #2158
Conversation
aa7bead
to
42985d1
Compare
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.
Some thoughts!
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.
lgtm
var jobMessage = await runServer.GetJobMessageAsync(messageRef.RunnerRequestId, messageQueueLoopTokenSource.Token); | ||
if (string.IsNullOrEmpty(messageRef.RunServiceUrl)) | ||
{ | ||
var actionsRunServer = HostContext.CreateService<IActionsRunServer>(); |
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.
These should be HostContext.GetService<>
so the underline object will be reused.
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 didn't do this btw, this was his how it was before 🤷🏻♂️ I added new code doing the same
Do you want me to do a follow up PR with just this change or do we have any other PR's that we can bundle this with?
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.
maybe just a follow-up PR for this change only.
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.
will do, thanks!
* changes to support run service url * feedback
* changes to support run service url * feedback
Issue: https://github.com/github/c2c-actions-service/issues/3231
Please see the list of all PR's involved and diagram showing before & after on high level and demo video at https://github.com/github/c2c-actions-service/issues/3231#issuecomment-1258397487
TL;DR: Run stamp will inject a run service URL as part of job message, which the broker would send that through to runner.
Runner would directly call the stamp to acquire job (no more proxying through actions service), so changes were needed to support that.
We didn't want to use vssf for the new call, so did the minimum to support the new calls, mimicking vssf but with out vssf based dependencies