-
Notifications
You must be signed in to change notification settings - Fork 148
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 container paths config #2340
Fix container paths config #2340
Conversation
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!
Is there an issue we can link this to? I assume this is a follow up from #2330 for #2315? I want to make sure we know what bug this is fixing so we can make sure it gets tested properly. If it's linked to #2315 then the test is that Heartbeat is able to start under the conditions described in that issue. |
🌐 Coverage report
|
There's no issue per se that this PR fixes, at least not an issue that's been filed in GitHub. I noticed the incorrect struct tags as part of investigating #2315. But the actual fix for #2315 is #2330. |
Thanks, no opposition to the change I just want to understand any intended (or unintended) consequences. |
Thanks for calling this out. I did some testing as well as digging into the PR that introduced the From reading that PR and the issue it fixed (elastic/beats#24956), I understand why the file is used for persisting the state path - so that commands like Put another way, after I started up an Elastic Agent container and deleted the @blakerouse Can you help answer the above question? |
@blakerouse @ycombinator bumping this just to avoid stalle PRs |
@ycombinator I provided some input over Slack, but didn't add it to this PR. I took another look at this and I believe that the The |
f7eaebe
to
da3713d
Compare
Thanks @blakerouse. I started to test this PR by comparing the outputs of |
da3713d
to
0fd4a6c
Compare
@ycombinator do we need another round of review here? |
@pierrehilbert Not yet. It's on my plate to test the changes in this PR; I just haven't gotten around to it yet (this PR is lower priority than some of the other things on my plate). |
89f7480
to
e8a42a7
Compare
/test |
I got around to testing this PR again, as described above. I see no differences when running the three commands using a build from this PR vs. using a build from I also tested running the Agent as a Docker container, pausing/unpausing it and stopping/starting it. I did this with the build from the PR as well as the build from So I don't believe this PR will introduce any intended or unintended consequences. |
* Fix config struct tags for containerPaths * Add unit test * Adding CHANGELOG file * Adding PR to CHANGELOG entry file * Running mage fmt (cherry picked from commit d7adf66)
* Fix config struct tags for containerPaths * Add unit test * Adding CHANGELOG file * Adding PR to CHANGELOG entry file * Running mage fmt (cherry picked from commit d7adf66) Co-authored-by: Shaunak Kashyap <[email protected]>
What does this PR do?
This PR fixes the parsing of paths from the
container-paths.yml
file used internally whenelastic-agent container
is run.Why is it important?
So the expected paths are loaded from
container-paths.yml
.Checklist
./changelog/fragments
using the changelog toolAuthor's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs