Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove --path.install option #2489

Closed
ycombinator opened this issue Apr 12, 2023 · 8 comments · Fixed by #6461
Closed

Remove --path.install option #2489

ycombinator opened this issue Apr 12, 2023 · 8 comments · Fixed by #6461
Assignees
@kaanyalti
Labels
Team:Elastic-Agent Label for the Agent team

Comments

@ycombinator
Copy link
Contributor

As per the conversation starting from https://github.com/elastic/ingest-dev/issues/1609#issuecomment-1503384831, the --path.install option is no longer being used by Elastic Agent. We should remove it but, since that would be a breaking change, we should do this in 9.0.

Copying relevant bits of the conversation here for easy reference:

@blakerouse said:

I believe with the changes in the V2 architecture (aka 8.6+) we missed removing some command-line arguments. --path.install has a different meaning then what this issue is seeking to solve. It is documented as Install path contains binaries Agent extracts, which meant the binaries that Elastic Agent extracted to run (aka. filebeat, metricbeat, etc..). Obviously with V2 (8.6+) we no longer extract the binaries they come pre-extracted so we no longer need or use the --path.install.

@ycombinator asked:

Shouldn't we wait to remove the --path.install option in 9.0 since it would technically be a breaking change to remove it in 8.x?

@jlind23 confirmed:

Let's wait for [9.0] indeed.
@ycombinator ycombinator added the Team:Elastic-Agent Label for the Agent team label Apr 12, 2023
@cmacknz
Copy link
Member

cmacknz commented Apr 19, 2023

Haven't we already made the breaking change by ignoring the argument? Anyone relying on this has already had their use case broken.

@blakerouse
Copy link
Contributor

@cmacknz No because you can still provide the --path.install to the Elastic Agent and it won't error. If we remove it then it would be breaking as the Elastic Agent would fail to start. We also didn't break any use-case because we no-longer extract to any directory so its not needed.

@belimawr
Copy link
Contributor

What about we at least edit the description of the flag to state it is deprecated and ignored?

On v8.11.1 (the latest release at the time of writting) we still have:

      --path.install string          Install path contains binaries Agent extracts

We could edit the message to something like "DEPRECATED, setting this flag has no effect". Or even add the version: "DEPRECATED, setting this flag has no effect since v8.6.0".

That way we do not break anything, keep the current behaviour and better inform the users about the current behaviour.

@cmacknz
Copy link
Member

cmacknz commented Nov 30, 2023

@belimawr good idea, go ahead and a make PR for it.

@belimawr
Copy link
Contributor

belimawr commented Dec 5, 2023

PR created: #3863

@jlind23
Copy link
Contributor

jlind23 commented Dec 30, 2024

@kaanyalti any draft PR you can link here?

@kaanyalti
Copy link
Contributor

@jlind23 Will create a draft early tomorrow morning for this.

@kaanyalti kaanyalti mentioned this issue Dec 31, 2024
Merged
3 tasks
@kaanyalti
Copy link
Contributor

@jlind23 here's the PR #6461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kaanyalti kaanyalti

Labels
Team:Elastic-Agent Label for the Agent team
Projects
None yet
Milestone
No milestone
6 participants
@ycombinator @belimawr @cmacknz @blakerouse @kaanyalti @jlind23