-
Notifications
You must be signed in to change notification settings - Fork 143
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
Show installation progress #3338
Conversation
🌐 Coverage report
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
We discussed this on Monday, thanks I'm sure it will help users to see that something is happening there! |
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.
Looks good. Simple design and easy to follow.
// Randomize interval between 65% and 250% of configured interval | ||
// to make it look like the progress is non-linear. :) | ||
floor := int64(math.Floor(float64(pt.tickInterval.Milliseconds()) * 0.65)) | ||
ceiling := int64(math.Floor(float64(pt.tickInterval.Milliseconds()) * 2.5)) |
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.
Haha, I don't know if this is really needed but okay! Why not.
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 whole deal is just to show some random progress to make things appear more realistic ;)
4880210
to
be1206b
Compare
buildkite test this |
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.
A wee blocker, isn't the 1st test susceptible to flakiness?
Another thing, it won't work well with nested steps, right?
|
||
pt.Stop() | ||
|
||
require.Equal(t, "step 1 starting... FAILED\n", string(w.buf)) |
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.
[Blocker]
Isn't there a chance there will be more than 3 dots(...
) when the assertion happens?
You didn't disable the randomised ticker intervals.
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.
We shouldn't see more than three dots because the step starts (line 36) and immediately fails (line 37). Unlike some of the other tests in this PR, there is no delay (sleep) in between those two lines, so more dots will not be shown.
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.
Please check my previous comment
Correct, if/when we have such steps or want to represent such steps in the progress, we'll need to enhance the |
/test |
SonarQube Quality Gate |
Unrelated failures, I'm merging |
What does this PR do?
This PR enhances
elastic-agent install
to show users the progress of the installation.Before this PR
before.mov
After this PR
after.mov
Why is it important?
So users know installation is proceeding and not stalled.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files./changelog/fragments
using the changelog toolI have added an integration test or an E2E test