feat : add run feature for trello provider #592
Conversation
| if err != nil { | ||
| return nil, fmt.Errorf("pull: %w", err) | ||
| } | ||
| tasks, err = dvstore.LoadTasks(s.h, s.schema, filters, s.opts.Logger) | ||
| tasks, err = dvstore.LoadTasks(s.h, s.schema, s.opts.TrelloToken, s.opts.TrelloApiKey, filters, s.opts.Logger) |
There was a problem hiding this comment.
Why do you need Token & API KEY for loading from the cayley store ?
There was a problem hiding this comment.
Yup, this should not happen.
But maybe it's due to a bad design decision I made earlier?
@AbdelkarimBENGRINE can you tell us more?
There was a problem hiding this comment.
this also surprised me but when I read Trello Api Doc
It is said that you have to provide both an api key and token unlike classic Oauth provider. I will look for implementation alternative if it causes too much problem in Depviz
| if err != nil { | ||
| return nil, fmt.Errorf("pull: %w", err) | ||
| } | ||
| } | ||
|
|
||
| var tasks dvmodel.Tasks | ||
| tasks, err = dvstore.LoadTasks(s.h, s.schema, filters, s.opts.Logger) | ||
| tasks, err = dvstore.LoadTasks(s.h, s.schema, s.opts.TrelloToken, s.opts.TrelloApiKey, filters, s.opts.Logger) |
There was a problem hiding this comment.
same answer as above :)
| paths = append(paths, p) | ||
|
|
||
| case multipmuri.TrelloProvider: | ||
| cardsId, err := trello.GetCardsId(target.LocalID()[1:], trelloToken, trelloApiKey) |
There was a problem hiding this comment.
Seem's weird to me to use Trello API to get informations for retrieve data on local store.
Is it the only way ?
There was a problem hiding this comment.
Yep same, but once again, it may come from a design limitation that I introduced before.
Maybe it makes sense to support lazy fetches at some points.
internal/dvcore/run.go
Outdated
| case multipmuri.TrelloProvider: | ||
| go func(board multipmuri.Entity) { | ||
| defer wg.Done() | ||
| trelloprovider.FetchCard(ctx, board, trelloToken, trelloApiKey, target.LocalID()[1:], out) |
There was a problem hiding this comment.
you should check target.LocalID() before calling [1:] in case it's empty.
I think it deserves a dedicated helper.
internal/dvcore/run.go
Outdated
| case multipmuri.TrelloProvider: | ||
| go func(board multipmuri.Entity) { | ||
| defer wg.Done() | ||
| trelloprovider.FetchCard(ctx, board, trelloToken, trelloApiKey, target.LocalID()[1:], out) |
There was a problem hiding this comment.
Btw, from pullBatches, I expect my drivers to fetch not only Trello Card but also Trello Board.
The helper here should be named trelloprovider.FetchTarget or something more flexible I guess.
| } | ||
|
|
||
| card := dvmodel.Task{ | ||
| ID: quad.IRI(entity.String()), |
There was a problem hiding this comment.
I'm not sure that entity.String is always unique. Maybe we can point to the same trello item with two different URLs.
Can you check in the input if there is no official URL instead maybe?
clean code Co-authored-by: Isma <[email protected]>
fix : clean code Co-authored-by: Isma <[email protected]>
fix : clean order lines Co-authored-by: Isma <[email protected]>
fix : reorder next provider Co-authored-by: Mikael VALLENET <[email protected]>
fix : comment non-provided fields Co-authored-by: Manfred Touron <[email protected]>
┆Issue is synchronized with this Trello card