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

[filebeat] Fix long filepaths in diagnostics exceeding max path limits on Windows #40909

Merged
merged 9 commits into from
Sep 20, 2024
Merged

[filebeat] Fix long filepaths in diagnostics exceeding max path limits on Windows #40909

merged 9 commits into from
Sep 20, 2024

Conversation

aleksmaus
Copy link
Member

Proposed commit message

Fix long filepaths in diagnostics exceeding max path limits on Windows.

Following the first proposal here
elastic/elastic-agent#3758 (comment)
Added IDWithoutName that doesn't include the source name and using that instead.

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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Screenshots

The screenshot that shows the diagnostics collected before and after this change. The earlier diagnostics shows the long path that includes the sanitized okta log collection URL. The diagnostics after this change have shorter path that doesn't include the sanitized URL in the path.
Screenshot 2024-09-19 at 2 36 12 PM

@aleksmaus aleksmaus added Filebeat Filebeat backport-skip Skip notification from the automated backport with mergify bugfix Team:Security-Deployment and Devices Deployment and Devices Team in Security Solution labels Sep 19, 2024
@aleksmaus aleksmaus self-assigned this Sep 19, 2024
@aleksmaus aleksmaus requested review from a team as code owners September 19, 2024 18:47
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Sep 19, 2024
@aleksmaus
Copy link
Member Author

This is odd linter complain to the line:

beats/x-pack/filebeat/input/cel/input.go

Line 1253 in 43d80af

key, rest, more := strings.Cut(path, ".") 

  Error: x-pack/filebeat/input/cel/input.go:1253:7: rest declared and not used (typecheck)
  	key, rest, more := strings.Cut(path, ".")

Can't reproduce this on my machine and the rest variable is clearly used later in this function

@efd6
Copy link
Contributor

efd6 commented Sep 19, 2024

@aleksmaus This is #39589.

efd6
efd6 reviewed Sep 19, 2024
View reviewed changes
x-pack/filebeat/input/cel/input.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/cel/input_test.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/httpjson/input_test.go Outdated Show resolved Hide resolved
@aleksmaus
Copy link
Member Author

aleksmaus commented Sep 19, 2024 •
edited
Loading

@aleksmaus This is #39589.

So what is the course of action in order to get this change in, which is unrelated to all the linter complains that surfaced just because I had to touch these files?

@efd6
Copy link
Contributor

efd6 commented Sep 19, 2024

The action is no action. Linter happiness is not a requirement for merging.

@aleksmaus
Copy link
Member Author

The action is no action. Linter happiness is not a requirement for merging.

Thank you for clarification. I didn't know. It is different in different repositories. Some of the repos are blocking PR on linter errors. Will revert. Let the linter complain away then.

@aleksmaus aleksmaus requested a review from efd6 September 19, 2024 21:48
efd6
efd6 approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Thanks, this is good to have in.

@aleksmaus
Copy link
Member Author

@ycombinator @belimawr @AndersonQ Guys, I need 👍 on this one from one of the code owners team members. Pretty please? :)

@belimawr
Copy link
Contributor

@ycombinator @belimawr @AndersonQ Guys, I need 👍 on this one from one of the code owners team members. Pretty please? :)

I'm looking at it right now ;)

@belimawr belimawr added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 20, 2024
belimawr
belimawr reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

LGTM, but I have two small comments:

  1. Is the goal of this PR to ensure the max path limit is never going to be exceeded or is it to just reduce the input name used by some inputs?
  2. Unless it's a 9.0 breaking change, all PRs should be backported to 8.x. I've added the backport label.

@aleksmaus
Copy link
Member Author

aleksmaus commented Sep 20, 2024 •
edited
Loading

  1. Is the goal of this PR to ensure the max path limit is never going to be exceeded or is it to just reduce the input name used by some inputs?

According to the original issue, the goal was to reduce the pathing length. The other path parts are more or less have predictable length.

@aleksmaus
Copy link
Member Author

2. Unless it's a 9.0 breaking change, all PRs should be backported to 8.x. I've added the backport label.

Good to know. Thanks!

@AndersonQ
Copy link
Member

I don't wanna block, but I don't see any tests to ensure the right name is used on the files. Unless it'd be too complicated, could you add some?

@aleksmaus aleksmaus merged commit d82fe9a into elastic:main Sep 20, 2024
25 of 28 checks passed
mergify bot pushed a commit that referenced this pull request Sep 20, 2024
@aleksmaus @mergify
[filebeat] Fix long filepaths in diagnostics exceeding max path limit…
6c67e3f 
…s on Windows (#40909)

* [filebeat] Fix long filepaths in diagnostics exceeding max path limits on Windows

* Update changelog with PR number

* Adjust utz

* Quiet the linter false positive

* More linter pacification

* Fix linter complains for errors checks in okta.go

* Let the linter complain again

* Revert revert

(cherry picked from commit d82fe9a)
@mergify mergify bot mentioned this pull request Sep 20, 2024
Merged
6 tasks
aleksmaus added a commit that referenced this pull request Sep 24, 2024
@mergify @aleksmaus
[filebeat] Fix long filepaths in diagnostics exceeding max path limit…
4ad8bd1 
…s on Windows (#40909) (#40923)

* [filebeat] Fix long filepaths in diagnostics exceeding max path limits on Windows

* Update changelog with PR number

* Adjust utz

* Quiet the linter false positive

* More linter pacification

* Fix linter complains for errors checks in okta.go

* Let the linter complain again

* Revert revert

(cherry picked from commit d82fe9a)

Co-authored-by: Aleksandr Maus <aleksandr.maus@elastic.co>
@jlind23 jlind23 mentioned this pull request Sep 25, 2024
Merged
@ycombinator ycombinator mentioned this pull request Sep 25, 2024
Merged
@mergify mergify bot mentioned this pull request Sep 26, 2024
Merged
@andrewkroh andrewkroh mentioned this pull request Sep 30, 2024
Closed
6 tasks
@efd6 efd6 mentioned this pull request Oct 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efd6 efd6 efd6 approved these changes

@mauri870 mauri870 mauri870 approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

@belimawr belimawr belimawr approved these changes

@khushijain21 khushijain21 Awaiting requested review from khushijain21 khushijain21 is a code owner automatically assigned from elastic/elastic-agent-data-plane

Assignees

@aleksmaus aleksmaus

Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-skip Skip notification from the automated backport with mergify bugfix Filebeat Filebeat Team:Security-Deployment and Devices Deployment and Devices Team in Security Solution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Diagnostic] Folder and file naming length
7 participants
@aleksmaus @elasticmachine @efd6 @belimawr @AndersonQ @andrewkroh @mauri870