-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: solaris build compatibility #45105
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
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
Is there anything else I need to do to move this forward? |
I don't see any blockers, it is missing reviews from our team so we should be focusing on that next. |
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.
Thanks, the changes here look good. I'll unblock the CI next.
libbeat/autodiscover/providers/docker/docker_integration_test.go
Outdated
Show resolved
Hide resolved
@mauri870 I think I've fixed the link errors. |
I did a mixture of actual changes where they were straightforward and a few places I did No worries on the delay, I was pencils-down on the weekend anyway, so it didn't slow me down. |
/test |
/test |
/test |
@pierrehilbert I'd love to address whatever problems are here, but I'm at a bit of a loss. |
Remaining linter issues:
|
Thanks for fixing the pending issues! Unblocking the CI 🙏 |
I don’t understand why every time we rerun the ci-cd it develops new issues in files that were unchanged from the last run and had no warnings previously. Further, I don’t understand why these issues don’t reflect what I see when running mage LLC with GOOS=linux. Any recommendations on how to make this less of a drip? |
/test |
Sorry about that, CI can be quite picky at times. I'm looking at the failures and looks like you need to run
|
I don’t really have a problem with it being picky. Incomplete and not available are the issues. It seems to be failing fast on lint failures in one file and since I can’t reproduce reliably on my machine, I have to wait basically a day for another file; all this because I added build directives to files causing them to be in scope. if there’s a trick to getting the same output on my local system as we get from the CI linter, that’d be super helpful, so I could clean everything at once before committing. |
Looking at the build code, I can pinpoint the commands that CI uses: make -C libbeat check update
make check-no-changes And for the linter: golangci-lint run --new-from-patch=<(git diff main) --new=false --new-from-rev= --timeout=30m --whole-files This last one is a github action, so I'm guessing what it probably does, I'm not entirely sure. Give it a try, it should report the linter failures I posted on #45105 (comment). |
Interestingly, it doesn't. Looking at the docs for golangci-lint it looks like GOOS isn't going to be sufficient. I'll need to spin up a separate Linux dev environment to run it in order to get the results from the linux version. That'll take me a day or two... |
/test |
The remaining linter failures seems unrelated with your changes. I'll give another quick review and then merge the changes. Thank you for contributing to Beats! |
Proposed commit message
This is a change to allow beats (for now specifically filebeat and heartbeat, others likely coming in the future) to be compiled and run on SmartOS and other Solaris systems.
Mostly this is a matter of excluding from build requirements, but for a couple of tests we also needed to stub out the kubernetes provider (which should also be usable on AIX).
Tests are accomplished with the existing test cases.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
There should be no impact for non-Solaris/Illumos/SmartOS users
Author's Checklist
How to test this PR locally
This will require a local Solaris/SmartOS/Illumos environment to test.
Related issues
Use cases
Screenshots
Logs