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.

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

Diagnostics creates the necessary folder for -f argument #3340

Merged

Conversation

AndersonQ
Copy link
Member

What does this PR do?

Fixes how the diagnostics command handles the custom path to save the diagnostics. Right now if the custom file path to save the diagnostics on contains folders that do not exist, the diagnostics fail to create the file, consequently failing to save the diagnostics.

Why is it important?

It not clear on the diagnostics help the path to the file must exist, it'd be much easier for our users if the diagnostics is capable of creating the necessary folders.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] 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 test

How to test this PR locally

Run the diagnostics command with the -f flag, passing a path where at least one folder does not exist. It will fail on main and succeed with this change.
E.g.:

sudo elastic-agent diagnostics -f /tmp/nonexisting/path/diag.zip
Error: open /tmp/nonexisting/path/diag.zip: no such file or directory

Related issues

Logs

sudo elastic-agent diagnostics -f /tmp/nonexisting/path/diag.zip
Error: open /tmp/nonexisting/path/diag.zip: no such file or directory

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@AndersonQ AndersonQ added bug Something isn't working Team:Elastic-Agent Label for the Agent team backport-v8.10.0 Automated backport with mergify labels Aug 31, 2023
@AndersonQ AndersonQ self-assigned this Aug 31, 2023
@AndersonQ AndersonQ requested a review from a team as a code owner August 31, 2023 15:36
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 31, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-01T05:09:54.528+0000

  • Duration: 27 min 37 sec

Test stats 🧪

Test Results
Failed 0
Passed 6249
Skipped 55
Total 6304

💚 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!)

 - makes diagnostics command to create the necessary folders when a custom path is provided by the '-f' flag.
 - refactors the diagnosticCmd splitting it into smaller functions
 - add test  createFile
 - rename logs_test.go createFile function to createFileEmpty so its name wont collide with the newly created 'createFile' function.
add license headaer
@AndersonQ AndersonQ force-pushed the 3339-fix-diagnostics-custom-path branch from 26f6494 to cdcd1eb Compare September 1, 2023 05:09
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 1, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.765% (80/81) 👍
Files 67.719% (193/285) 👍 0.351
Classes 66.292% (354/534) 👍 0.187
Methods 53.014% (1108/2090) 👎 -0.003
Lines 38.623% (12648/32747) 👍 0.004
Conditionals 100.0% (0/0) 💚

Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment about the renaming for the logs, looks ok otherwise

I saw that the PR is labeled for 8.10 backport but we are already creating BCs for that release and this is not a blocker for 8.10. Do we want to backport it anyways ?

@@ -99,7 +99,7 @@ func TestGetLogFilenames(t *testing.T) {

t.Run("does not return non-log entries", func(t *testing.T) {
dir := t.TempDir()
createFile(t, dir, "excluded")
createFileEmpty(t, dir, "excluded")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking: has this been renamed because of name collision within the same package?

@AndersonQ AndersonQ enabled auto-merge (squash) September 1, 2023 11:54
@pierrehilbert
Copy link
Contributor

Failing tests are unrelated. I'm forcing the merge.

@pierrehilbert pierrehilbert merged commit 4cd83bf into elastic:main Sep 1, 2023
12 checks passed
mergify bot pushed a commit that referenced this pull request Sep 1, 2023
- makes diagnostics command to create the necessary folders when a custom path is provided by the '-f' flag.
 - refactors the diagnosticCmd splitting it into smaller functions
 - add test  createFile
 - rename logs_test.go createFile function to createFileEmpty so its name wont collide with the newly created 'createFile' function.
add license headaer

(cherry picked from commit 4cd83bf)
pierrehilbert pushed a commit that referenced this pull request Sep 1, 2023
- makes diagnostics command to create the necessary folders when a custom path is provided by the '-f' flag.
 - refactors the diagnosticCmd splitting it into smaller functions
 - add test  createFile
 - rename logs_test.go createFile function to createFileEmpty so its name wont collide with the newly created 'createFile' function.
add license headaer

(cherry picked from commit 4cd83bf)

Co-authored-by: Anderson Queiroz <[email protected]>
@AndersonQ AndersonQ deleted the 3339-fix-diagnostics-custom-path branch September 1, 2023 15:10
cmacknz pushed a commit to cmacknz/elastic-agent that referenced this pull request Sep 25, 2023
- makes diagnostics command to create the necessary folders when a custom path is provided by the '-f' flag.
 - refactors the diagnosticCmd splitting it into smaller functions
 - add test  createFile
 - rename logs_test.go createFile function to createFileEmpty so its name wont collide with the newly created 'createFile' function.
add license headaer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.10.0 Automated backport with mergify bug Something isn't working Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diagnostics fails when folders on custom path does not exist
5 participants