Skip to content

Conversation

@faec
Copy link
Contributor

@faec faec commented Mar 20, 2025

Reverts #43387

This PR is breaking the metricbeat packaging (https://buildkite.com/elastic/beats-xpack-metricbeat/builds/13512#0195b4ca-f2d5-44f7-8e12-486a28814556) and crosscompile (https://buildkite.com/elastic/beats-metricbeat/builds/15782#0195b4ea-77a7-482c-be10-f27ea95bd74f) tests. A dependency check bug seems to have skipped those tests (and most others) on the PR's CI, which made it appear green before merge.

@faec faec self-assigned this Mar 20, 2025
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 20, 2025
@botelastic
Copy link

botelastic bot commented Mar 20, 2025

This pull request doesn't have a Team:<team> label.

@faec faec marked this pull request as ready for review March 20, 2025 20:32
@faec faec requested a review from a team as a code owner March 20, 2025 20:32
@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @faec? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@faec faec marked this pull request as draft March 20, 2025 20:35
@v1v
Copy link
Member

v1v commented Mar 20, 2025

Wait, we are merging things, so there might be instability, but this should not be done now

@faec faec marked this pull request as ready for review March 20, 2025 20:43
@v1v v1v marked this pull request as draft March 20, 2025 20:44
@v1v
Copy link
Member

v1v commented Mar 20, 2025

We have probably forgot to say that we are working on it:

Status for Beats:

In your case the failure is about a PR in one of those branches.

Sorry for the noise, but we are actually working as we speak

@v1v
Copy link
Member

v1v commented Mar 20, 2025

d crosscompile (buildkite.com/elastic/beats-metricbeat/builds/15782#0195b4ea-77a7-482c-be10-f27ea95bd74f) tests.

That's in main, interestingly it has not been found in those PRs:

So we need some help so we can keep using the approach we have done so far:

  • refactor golang-crossbuild to support darwin/arm64 for Debian 11 on arm64
  • changes in Beats
  • changes in Elastic Agent

@faec
Copy link
Contributor Author

faec commented Mar 20, 2025

The crosscompile failures were in the main branch (on #43394), but I will rerun the tests there to see if they're working again now

@v1v
Copy link
Member

v1v commented Mar 20, 2025

The crosscompile failures were in the main branch (on #43394), but I will rerun the tests there to see if they're working again now

If it works in main:

Maybe it's something with your PR or maybe it's not up-to-date?

@faec
Copy link
Contributor Author

faec commented Mar 20, 2025

That's in main, interestingly it has not been found in those PRs:

@v1v The split PR looks like it didn't actually run those tests before merge, which I think was an error in the CI itself? But it does seem to have passed on its post-merge run in main. If it doesn't happen anymore then the rollback is unnecessary :-)

@v1v
Copy link
Member

v1v commented Mar 20, 2025

Thanks @faec for double-checking and so sorry for missing the way to communicate this!! 🥺

@v1v
Copy link
Member

v1v commented Mar 20, 2025

@v1v The split PR looks like it didn't actually run those tests before merge, which I think was an error in the CI itself?

There are two PRs:

  1. One related to the packaging on PRs.
  2. One related to the DRA.

In both cases, we changed in the golang-crossbuild the support for darwin/arm64 on arm64, that's an independent change and it was done about 2 hours ago (as a prerequisite for those PRs):

That change forced us to use darwin/arm64 for arm64 in the consumers, hence the changes in the above-mentioned PRs.

#43387 didn't test anything because for some reason the CI has not been configured to test changes in that pipeline, as far as I understood (why? no clue, that's something I have not been involved). However,
#43280 , that one ran the crosscompile for metricbeats:

I hope I clarified a bit the context, but If I missed something please let me know, just in case something else is not right! Thanks again @faec

@mergify
Copy link
Contributor

mergify bot commented May 20, 2025

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b revert-43387-feature/dra-packaging-split-arm64 upstream/revert-43387-feature/dra-packaging-split-arm64
git merge upstream/main
git push upstream revert-43387-feature/dra-packaging-split-arm64

@dliappis
Copy link
Contributor

@pkoutsovasilis / @v1 Is this something we can close?

@pkoutsovasilis
Copy link
Contributor

IMO this can be closed 🙂

@v1v v1v closed this May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_team Indicates that the issue/PR needs a Team:* label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants