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

Revert "bk(dra): split arm64 for linux and darwin from amd64" #43404

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

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
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

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.

2 participants