-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Resolve Actions Directly From Launch for Run Service Jobs #2529
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
Conversation
| public string Action { get; set; } | ||
|
|
||
| [DataMember(EmitDefaultValue = false, Name = "version")] | ||
| public string Version { get; set; } |
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 is currently called Ref instead of Version, what's the reason for the rename?
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 is copied pretty much directly from https://github.com/github/actions-dotnet/blob/179ed33568dd1ad80c457fb1c328c964e372accf/Launch/Sdk/Server/Models/GitHub.cs#L90-L152. Since the runner is now the one receiving the payload from launch, it needs to parse the JSON payload the same way. The same goes for sending the payload to Launch. The runner needs to serialize the data into JSON that Launch expects. I don't believe this is a rename
| public string Version { get; set; } | ||
|
|
||
| [DataMember(EmitDefaultValue = false, Name = "path")] | ||
| public string Path { get; set; } |
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.
Does the server care about the Path of the action?
| public string TarUrl { get; set; } | ||
|
|
||
| [DataMember(EmitDefaultValue = false, Name = "version")] | ||
| public string Version { get; set; } |
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.
same question about version vs ref
| public class ActionDownloadAuthenticationResponse | ||
| { | ||
| [DataMember(EmitDefaultValue = false, Name = "expires_at")] | ||
| public DateTime ExpiresAt { get; set; } |
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.
do we really care about the token's expiresAt?
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.
It's necessary for building the ActionDownloadAuthentication https://github.com/jherns/runner/blob/2bde42187e2373c2b1516f4df4b3c214e3e09e57/src/Sdk/WebApi/WebApi/LaunchHttpClient.cs#L106-L110
| } | ||
|
|
||
| [DataContract] | ||
| public class ActionReferenceRequestList |
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.
why do we need an object to wrap around a list?
Can we just send the List as json to service?
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.
The endpoint for resolving actions expects an object wrapped around a list
| } | ||
|
|
||
| [DataContract] | ||
| public class ActionDownloadInfoResponseCollection |
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.
same question, why do we need this wrapper around a dictionary?
Co-authored-by: Tingluo Huang <tingluohuang@github.com>
Co-authored-by: Tingluo Huang <tingluohuang@github.com>
Co-authored-by: Tingluo Huang <tingluohuang@github.com>
Co-authored-by: Tingluo Huang <tingluohuang@github.com>
LaunchHttpClientLaunchServerLaunchServerwithinJobServerQueueJobServerQueuewhen running Run Service JobsActionManager