Use third-party library for reporting install/uninstall progress #3623

fearful-symmetry · 2023-10-17T18:28:30Z

What does this PR do?

This uses a third-party library for creating a spinner that reports progress during install/uninstall operations.

Why is it important?

The old code was causing some flaky tests, and for such a small bit of code, it was a bit easier to just use a third-party library.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

elasticmachine · 2023-10-17T18:28:44Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

mergify · 2023-10-17T18:29:08Z

This pull request does not have a backport label. Could you fix it @fearful-symmetry? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2023-10-17T19:01:00Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-10-23T15:52:48.457+0000
Duration: 28 min 11 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6553
Skipped	59
Total	6612

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-10-17T19:01:09Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.824% (`84/85`)	👍
Files	67.105% (`204/304`)	👍
Classes	66.068% (`368/557`)	👎 -0.121
Methods	53.284% (`1160/2177`)	👎 -0.299
Lines	39.586% (`13657/34500`)	👎 -0.213
Conditionals	100.0% (`0/0`)	💚

ycombinator · 2023-10-17T19:48:10Z

At that point we're getting kind of complicated for what this is, and we might just want to find a third-party library like https://github.com/schollz/progressbar to just do this for us, which is also an option here. Honestly open to the third-party library idea for a follow-up to this; not sure why we're DIY'ing this.

I'm ++ to using a third-party library here. I've been thinking along the same lines myself.

The initial implementation was done as one of three OnWeek projects (so, pretty quickly) and, at the time, I thought it would be simpler to DIY this especially since we weren't doing anything too fancy like repainting the screen (like some third party progress trackers can do). I honestly hadn't thought it would introduce so much flakiness and headache. But after seeing that over the past few weeks, I'm very much in favor of just replacing this with a third-party library at this point.

…ace-cond-check-progress

fearful-symmetry · 2023-10-18T21:46:28Z

Alright, so, it looks like SonarQube is counting all the one-line log statements I added towards the code coverage percentage, which is...not great.

ycombinator · 2023-10-19T05:49:29Z

I like the new progress bar and the time counter too!

There are two things I was wondering if we could change:

Would it be possible to display all the steps? That is, when a step completes, would it be possible to leave it on the screen without it being replaced by the next step?

The install ends like so:

$ sudo ./elastic-agent install
Elastic Agent will be installed at /Library/Elastic/Agent and will run as a service. Do you want to continue? [Y/n]:y
Do you want to enroll this Agent into Fleet? [Y/n]:n
[=   ] Service Started  [11s] Elastic Agent has been successfully installed.

The looks a bit odd. I think we're missing a newline?

fearful-symmetry · 2023-10-19T15:38:15Z

@ycombinator added the newline. However, printing each message to a newline isn't quite something we can trivially do.

ycombinator · 2023-10-19T16:16:32Z

Right after sudo ./elastic-agent install is called, there's a spinner but no text is shown next to it. I managed to capture a screenshot of the moment:

Seems like there should be some text there about the work being done?

I'm not seeing the same behavior during uninstall, though, which is good.

fearful-symmetry · 2023-10-19T16:51:53Z

@ycombinator wow, that happens too fast on my machine to even notice. Should be fixed now.

ycombinator · 2023-10-19T19:40:41Z

Thanks @fearful-symmetry, I see the initial message during sudo elastic-agent install now.

One nit:

During uninstallation (sudo elastic-agent uninstall), when uninstallation succeeds, the last progress message that's printed is "Done".

$ sudo elastic-agent uninstall -f
[ ===] Done  [1s]
Elastic Agent has been uninstalled.

However, during installation (sudo ./elastic-agent install), when installation succeeds, the last progress message that's printed is the last actual installation step.

$ sudo ./elastic-agent install -n
Installing in non-interactive mode.
[==  ] Service Started  [11s]
Elastic Agent has been successfully installed.

Could we make these behaviors consistent? I'm not sure which direction makes more sense but I'm leaning slightly towards the way uninstallation behaves.

ycombinator

LGTM. Thanks for fixing this, Alex — much needed!

mergify · 2023-10-23T07:27:39Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b race-cond-check-progress upstream/race-cond-check-progress
git merge upstream/main
git push upstream race-cond-check-progress

pchila · 2023-10-23T12:18:16Z

internal/pkg/agent/cmd/install.go

@@ -51,7 +51,7 @@ would like the Agent to operate.
 func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error {
 	err := validateEnrollFlags(cmd)
 	if err != nil {
-		return err
+		return fmt.Errorf("could not validate flags: %w", err)


pchila

LGTM

…gress

elastic-sonarqube · 2023-10-23T16:04:44Z

SonarQube Quality Gate

9.5% Coverage on New Code (is less than 40%)

See analysis details on SonarQube

cmacknz · 2023-10-25T16:04:11Z

I am going to force merge this. Looking at the code that is missing coverage, some of it is covered by integration tests and I don't see value in simulating the 10+ error conditions we can hit just to verify a log line gets printed as expected.

orestisfl · 2023-11-28T16:48:08Z

Just noticed this on a snapshot and it looks amazing 🤩 👍

fearful-symmetry added 2 commits October 16, 2023 13:41

tinker with mutexies in writer

6a3896a

add changelog, flag variable

a79c36c

fearful-symmetry requested a review from a team as a code owner October 17, 2023 18:28

fearful-symmetry requested review from michalpristas and pchila October 17, 2023 18:28

fearful-symmetry self-assigned this Oct 17, 2023

fearful-symmetry added the Team:Elastic-Agent Label for the Agent team label Oct 17, 2023

mergify bot added the backport-skip label Oct 17, 2023

use third-party spinner library

82a2521

fearful-symmetry changed the title ~~More aggressive mutexes and flags in progress tracker~~ Use third-party library for reporting install/uninstall progress Oct 18, 2023

fearful-symmetry and others added 6 commits October 18, 2023 13:25

update changelog

fdc61c4

linter

879c198

update notice

c54fe98

try notice again

25f366c

add test to make linter happy

d729dee

Merge remote-tracking branch 'origin/race-cond-check-progress' into r…

9c8ac4f

…ace-cond-check-progress

pierrehilbert requested a review from ycombinator October 19, 2023 05:31

add newline to install message

5111dd3

add initial message

c63ebfb

add done message

eb43d3a

ycombinator approved these changes Oct 19, 2023

View reviewed changes

pchila reviewed Oct 23, 2023

View reviewed changes

pchila approved these changes Oct 23, 2023

View reviewed changes

fearful-symmetry and others added 2 commits October 23, 2023 08:22

Merge remote-tracking branch 'upstream/main' into race-cond-check-pro…

3873188

…gress

update notice

1f01a7d

michalpristas approved these changes Oct 24, 2023

View reviewed changes

cmacknz merged commit 13e1fe4 into elastic:main Oct 25, 2023
7 of 8 checks passed

cmacknz mentioned this pull request Oct 27, 2023

Failing to detect SSDs in copyDir should not be a fatal error. #3653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use third-party library for reporting install/uninstall progress #3623

Use third-party library for reporting install/uninstall progress #3623

fearful-symmetry commented Oct 17, 2023 •

edited

Loading

elasticmachine commented Oct 17, 2023

mergify bot commented Oct 17, 2023

elasticmachine commented Oct 17, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Oct 17, 2023 •

edited

Loading

ycombinator commented Oct 17, 2023 •

edited

Loading

fearful-symmetry commented Oct 18, 2023

ycombinator commented Oct 19, 2023

fearful-symmetry commented Oct 19, 2023

ycombinator commented Oct 19, 2023

fearful-symmetry commented Oct 19, 2023

ycombinator commented Oct 19, 2023

ycombinator left a comment

mergify bot commented Oct 23, 2023

pchila Oct 23, 2023

pchila left a comment

elastic-sonarqube bot commented Oct 23, 2023

cmacknz commented Oct 25, 2023

orestisfl commented Nov 28, 2023

Use third-party library for reporting install/uninstall progress #3623

Use third-party library for reporting install/uninstall progress #3623

Conversation

fearful-symmetry commented Oct 17, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Oct 17, 2023

mergify bot commented Oct 17, 2023

elasticmachine commented Oct 17, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Oct 17, 2023 • edited Loading

🌐 Coverage report

ycombinator commented Oct 17, 2023 • edited Loading

fearful-symmetry commented Oct 18, 2023

ycombinator commented Oct 19, 2023

fearful-symmetry commented Oct 19, 2023

ycombinator commented Oct 19, 2023

fearful-symmetry commented Oct 19, 2023

ycombinator commented Oct 19, 2023

ycombinator left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 23, 2023

pchila Oct 23, 2023

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Oct 23, 2023

cmacknz commented Oct 25, 2023

orestisfl commented Nov 28, 2023

fearful-symmetry commented Oct 17, 2023 •

edited

Loading

elasticmachine commented Oct 17, 2023 •

edited

Loading

elasticmachine commented Oct 17, 2023 •

edited

Loading

ycombinator commented Oct 17, 2023 •

edited

Loading