WIP: Create APIS import task and wait for status#180
WIP: Create APIS import task and wait for status#180jraddaoui wants to merge 1 commit intodev/issue-173-apis-mockfrom
Conversation
|
Just missing the OIDC provider configuration and setup, similar to what we do in Enduro with the gotools/clientauth package. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/issue-173-apis-mock #180 +/- ##
===========================================================
- Coverage 61.23% 60.79% -0.44%
===========================================================
Files 43 47 +4
Lines 2515 2760 +245
===========================================================
+ Hits 1540 1678 +138
- Misses 830 931 +101
- Partials 145 151 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add generated APIS API client. - Add initial APIS API client config and implementation. - Add APIS activities to create import task and poll status. - Create preprocessing tasks for both activities.
jraddaoui
force-pushed
the
dev/issue-173-apis-import-task
branch
from
f84e56f to
f84fc5b
Compare
djjuhasz
left a comment
djjuhasz
left a comment
There was a problem hiding this comment.
Looking good @jraddaoui. I had a handful of minor notes, but the structure looks good. I think it's a really good decision to add a config gate for the APIS functionality so the workflow can run without communicating with the APIS server if necessary.
| if c.Timeout < 0 { | ||
| return fmt.Errorf("invalid APIS timeout: %s", c.Timeout) | ||
| } | ||
| if c.PollInterval <= 0 { |
There was a problem hiding this comment.
Why aren't you setting the PollInterval to the DefaultPollInterval when the config value is 0, like you do for the Timeout?
| return nil, fmt.Errorf("create APIS import task: %w", err) | ||
| } | ||
|
|
||
| switch res := res.(type) { |
There was a problem hiding this comment.
Shadowing the res variable.
| )) | ||
| } | ||
| return &CreateImportTaskResult{TaskID: taskID}, nil | ||
| case *gen.APIImportTasksPostBadRequest: |
There was a problem hiding this comment.
Wow, ogen creates a new error type for every error response status code, that's just an alias for gen.CreateImportTaskResponse?! I guess so we can do this kind of type checking? It seems like it would be easier to just check a statusCode value. 😕
It would be nice if ogen also defined a common interface so we could do something like:
func createImportTaskResponseError(r gen.ErrorGetter) string {
err, ok := r.GetError().Get()
...
}| ) (*CreateImportTaskResult, error) { | ||
| metadata, err := os.Open(params.SIP.MetadataPath) | ||
| if err != nil { | ||
| return nil, temporal.NewNonRetryableError(fmt.Errorf("open metadata.xml: %v", err)) |
There was a problem hiding this comment.
Hmm, I should probably be using temporal.NewNonRetryableError() more. 🤔
|
|
||
| func NewPollImportTaskStatusActivity(client Client, pollInterval time.Duration) *PollImportTaskStatusActivity { | ||
| if pollInterval <= 0 { | ||
| pollInterval = 30 * time.Second |
There was a problem hiding this comment.
I think this should use apis.DefaultPollInterval.
| Temporal.TaskQueue: missing required value | ||
| Temporal.WorkflowName: missing required value`, | ||
| Temporal.WorkflowName: missing required value | ||
| missing APIS URL`, |
There was a problem hiding this comment.
I think this validation error should follow the pattern of the other error messages, i.e. APIS.URL: missing required value
| `, | ||
| wantFound: true, | ||
| wantErr: `invalid configuration | ||
| missing APIS URL`, |
There was a problem hiding this comment.
Should be APIS.URL: missing required value
| `, | ||
| wantFound: true, | ||
| wantErr: `invalid configuration | ||
| invalid APIS timeout: -1s`, |
There was a problem hiding this comment.
Should be: APIS.Timeout: value -1s is less than the minimum value of 0 (or similar)
| `, | ||
| wantFound: true, | ||
| wantErr: `invalid configuration | ||
| invalid APIS poll interval: -1s`, |
There was a problem hiding this comment.
Should be: APIS.PollInterval: value -1s is less than the minimum value of 0 (or similar)
| apis.CreateImportTaskActivityName, | ||
| &apis.CreateImportTaskParams{ | ||
| SIP: sip, | ||
| Username: "preprocessing-sfa", // TODO: Use real username. |
There was a problem hiding this comment.
Don't forget to update this. ;)
2 participants
Refs #173. Based in #179.