[CLC-426]: Show Overall Remaining Time and Completion Percentage #423
Conversation
I think it is a bit hard to test it, because values are changing constantly. And it is very easy to detect with manual tests that it wouldn't worth the effort IMO. |
|
I was talking about maybe running dmt estimate and checking the output has the "remaining time x seconds" text. (via regex) @kutluhanmetin but it's not too important if it's difficult |
| // single iteration is enough that we are reading single result for a single migration | ||
| row, err := it.Next() | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } |
There was a problem hiding this comment.
I thought you've introduced a function to query SQL and return a single result
There was a problem hiding this comment.
You are referring to querySingleRow that only uses row.Get(0) to fetch single result. But in here I have two values to read that I could not reuse it.
There was a problem hiding this comment.
Maybe querySingleRow could be changed to return the row, not the first column.
There was a problem hiding this comment.
The other functions use that function in common, so it is re-used a couple of times. Shall we keep it as is?
There was a problem hiding this comment.
If it's not too hard to make that change, I would change function I've described and change the code where it is called.
There was a problem hiding this comment.
I didn't mean to say it is hard, I think it is not very useful.
There was a problem hiding this comment.
There are only 4 places querySingleRow is used, so not much code will be changed with that refactoring, and it will be more generic, so it can also be used ^.
You can also change its return value to return []any to return all column values.
| status.SetText("Unable to calculate remaining duration and progress") | ||
| } else { | ||
| status.SetProgress(cp) | ||
| status.SetRemainingDuration(rt) |
There was a problem hiding this comment.
What happens if the progress status is received after setting the ""Unable to calculate remaining duration and progress" text? Is it changed to "XXX is in progress?
There was a problem hiding this comment.
It is a bit hard to run this scenario on my local environment. I was expecting it to be updated with the stage's ProgressMsg. Is it wrong?
There was a problem hiding this comment.
No, progress message is set once, before the stage starts executing, see: clc/ux/stage/stage.go:152
There was a problem hiding this comment.
ok, this commit fixes it: 465a2e0
| // single iteration is enough that we are reading single result for a single migration | ||
| row, err := it.Next() | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } |
There was a problem hiding this comment.
If it's not too hard to make that change, I would change function I've described and change the code where it is called.
# Conflicts: # base/commands/migration/migration_stages.go
No description provided.