-
Notifications
You must be signed in to change notification settings - Fork 141
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
Split install and enroll on test framework #4482
Conversation
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
4e5fb1b
to
f0a2fcf
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
Multiple linter issues and some requests from my side.
// check for running agents before installing, there should be no agent | ||
// process running. However, even if there is one, let's keep the test going | ||
// and add a failure so we can investigate what happened. | ||
// assert.Empty(f.t, getElasticAgentProcesses(f.t), "there should be no running agent at beginning of Install()") |
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.
why is this commented now?
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.
as I commented in PR, it's still in draft, sorry I forgot to set it as draft when opening the PR
if err != nil && | ||
(errors.Is(err, ErrNotInstalled) || | ||
strings.Contains( | ||
err.Error(), | ||
"elastic-agent: no such file or directory")) { |
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.
Since we're already refactoring, please assign each condition to a variable, this is very hard to read.
if err != nil { | ||
f.t.Logf("error serializing processes: %s", err) | ||
} | ||
if f.cleanUpKeepInstalled() { |
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 returns a boolean which is not documented, I cannot understand from the function name what this boolean is.
|
||
f.t.Cleanup(func() { | ||
if f.cleanUpUninstall() { |
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.
Same about boolean here.
@@ -447,10 +447,20 @@ func (r *Runner) validate() error { | |||
|
|||
// getBuilds returns the build for the batch. | |||
func (r *Runner) getBuilds(b OSBatch) []Build { | |||
builds := []Build{} | |||
var builds []Build | |||
formats := []string{"targz", "zip", "rpm", "deb"} |
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.
Since we started using slice.Contains
on this, why not define a global map[string]struct{}
and just look strings up instead? It's way more efficient than iterating through a slice on each loop iteration.
@AndersonQ Seems like enroll does not work in integration tests now. See the CI build. |
my bad, it isn't ready fore review yet. I set it as draft again |
This pull request is now in conflicts. Could you fix it? 🙏
|
It's quite old, closing it for now |
What does this PR do?
It splits the install and enroll command on the test framework
Fixture.Install
It also refactors the clean up function
Why is it important?
If any error happens during the agent, it'll automatically uninstall itself, which whereas good for production, prevent us from investigating what might have caused the error. It's specially important when the error happens during enroll.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] 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 testHow to test this PR locally
Related issues
Questions to ask yourself