feat: add optimus replay list #25
Conversation
odpf/optimus/runtime_service.proto
Outdated
| @@ -194,6 +194,11 @@ service RuntimeService { | |||
| get: "/v1/project/{project_name}/job/{job_name}/replay/{id}" | |||
| }; | |||
| } | |||
| rpc GetReplayList(GetReplayListRequest) returns (GetReplayListResponse) { | |||
| option (google.api.http) = { | |||
| get: "/v1/project/{project_name}/replay/list" | |||
There was a problem hiding this comment.
we should make it follow REST principles, do we need list?
There was a problem hiding this comment.
updated to not use list
odpf/optimus/runtime_service.proto
Outdated
| @@ -194,6 +194,11 @@ service RuntimeService { | |||
| get: "/v1/project/{project_name}/job/{job_name}/replay/{id}" | |||
| }; | |||
| } | |||
| rpc GetReplayList(GetReplayListRequest) returns (GetReplayListResponse) { | |||
There was a problem hiding this comment.
ReplayList sounds better to me, but you have concern on this? And should we change the GetReplayStatus too?
There was a problem hiding this comment.
are we considering replay as a resource? In that case, I guess it should be replays in endpoint and ListReplays in the method.
There was a problem hiding this comment.
replays seems more descriptive. but i'm seeing other endpoints are not in a plural word, for example when getting projects or resource specs. thoughts?
There was a problem hiding this comment.
understood why ListReplays is better, just went through the method naming convention.
There was a problem hiding this comment.
seems Optimus is using singular in all the endpoint, do you think it is acceptable to keep as it is @ravisuhag ? if not, we need to apply the changes to the others as well to keep it in uniform. cc @kushsharma
There was a problem hiding this comment.
Planning to resolve this and thinking to just follow the current Optimus convention to use singular, and if a change in convention is needed, will handle it in a different issue. Please let me know if you have any concern on this, @ravisuhag
odpf/optimus/runtime_service.proto
Outdated
| string id = 1; | ||
| string job_name = 2; | ||
| string start_date = 3; |
There was a problem hiding this comment.
why not google.protobuf.Timestamp
There was a problem hiding this comment.
i'm fine either way, but since start and end date is a date, i figured the response we are sending is already represented as expected, not in protobuf timestamp. any concern on this?
There was a problem hiding this comment.
discussed this and decided to use timestamp
odpf/optimus/runtime_service.proto
Outdated
| @@ -179,24 +179,24 @@ service RuntimeService { | |||
| } | |||
| rpc ReplayDryRun(ReplayRequest) returns (ReplayDryRunResponse) { | |||
| option (google.api.http) = { | |||
| post: "/v1/project/{project_name}/job/{job_name}/replay-dry-run" | |||
| post: "/v1/project/{project_name}/replay/{job_name}/dryrun" | |||
There was a problem hiding this comment.
sinc dryrun is custom method. it can go as :dryrun or maybe just :dry ?
https://cloud.google.com/apis/design/custom_methods
There was a problem hiding this comment.
makes sense. let me change it to :dryrun and test it
There was a problem hiding this comment.
@ravisuhag using colon for identifying a custom verb is one way of doing it, other way is to split these custom verbs via slash. The advantage we have here when not using :verb is it avoid conflicts with the resource name. For example
/v1/project/{project_name}/replay/{job_name}/dryrun
/v1/project/{project_name}/replay/{job_name}:dryrun
Also I wonder how will the gateway regex be able to differentiate that when a URL looks like
/v1/project/test-project/replay/test-job:dryrun
job_name is test-job:dryrun or test-job? So these colon based verbs are nice when we have a fixed endpoint for example like
/v1/project/{project_name}/replay:dryrun
and have everything in the POST payload but then we won't be able to do GET calls nicely.
Its very rare we will be going to allow a job name that contains a / whereas a job name can contain : now or in the future. These colon based verbs can be used if we split them after a slash though like
/v1/project/{project_name}/replay/{job_name}/:dryrun
But then this looks like more of a choice of convention. For example github follows without colon custom verbs. What do you think?
There was a problem hiding this comment.
Ok cool. fine with slash as well.
…method convention
… force from replay dry run
Add Optimus get replay list rpc & message models.