-
Notifications
You must be signed in to change notification settings - Fork 144
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
fix(5595): added logic to use development binary path for uninstall #5877
base: main
Are you sure you want to change the base?
fix(5595): added logic to use development binary path for uninstall #5877
Conversation
This pull request does not have a backport label. Could you fix it @kaanyalti? 🙏
|
|
0a1aca2
to
05882da
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
please check integration test failures, these are related |
devOut2, err := devFix2.Install(ctx, &devInstOpt) | ||
if err != nil { | ||
t.Logf("install output: %s", devOut2) | ||
require.NoError(t, err) |
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 will always fail if err is not nil
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.
I was following similar implementation in the prior test cases, but I can remove the if statement and just assert against the error.
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.
just in case you don't know assert.X
will just add a failure to the test, whereas require.X
will abort the test. So the idea is to use assert.X
for the actual test assertions and require.X
during setup to avoid the test continuing after critical failure
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.
I did not know that, thank you
565e893
to
3210915
Compare
…pment installation
9be1794
to
9d5c737
Compare
Quality Gate failedFailed conditions |
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.
I don't think this is taking the correct approach currently. I would expect the usage of the following would allow you to get the correct path to the binary that should be called to perform the uninstall.
func InstallNamespace() string { |
// DevelopmentBinaryName is the name of the installed binary when --develop | ||
// flag is used | ||
DevelopmentBinaryName = "elastic-development-agent" | ||
|
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.
I don't think you can do this, as the --develop
is just an alias for the --namespace
flag. That allows multiple agents to be installed on the same host at the same time. The name of the executable here should not be hard coded as it should either come from the --namespace
flag or from infering the namespace by reading the namespace from the path that is loaded using the full filepath from os.Args[0]
.
What does this PR do?
Updates how the
upgrade
command works when used with the--force
and--develop
flags. Currently if a development agent is already installed, and if a user wants to force install a new development agent, the command uninstalls the production agent, if it exists, and tries to install the development agent again.Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files./changelog/fragments
using the changelog toolHow to test this PR locally
--force
flag and confirm that at the end of the operation bothelastic-agent
andelastic-development-agent
binaries are installed and that both agents are healthyRelated issues
--force
ignores--develop
and uninstalls production agent instead #5595