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

Fix Conformance tests on ARM64 #5990

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

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Feb 14, 2025

What type of PR is this?

/kind bug
/kind failing-test

What this PR does / why we need it:

This PR fixes bugs discovered by running Conformance tests on ARM64.

Fixed tests:

  • TestConformance/setuid-file-in-archive
  • TestConformance/multistage-builtin-args
  • TestConformance/builtins

How to verify it

Execute Conformance tests on ARM system.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Copy link
Contributor

openshift-ci bot commented Feb 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Honny1
Once this PR has been reviewed and has the lgtm label, please assign umohnani8 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Honny1 Honny1 marked this pull request as ready for review February 17, 2025 11:55
@TomSweeneyRedHat
Copy link
Member

LGTM

Comment on lines 454 to 458
buildVariant := b.BuiltinArgDefaults["BUILDVARIANT"]
buildPlatform := b.BuiltinArgDefaults["BUILDPLATFORM"]
cleanedBuildPlatform, _ := strings.CutSuffix(buildPlatform, "/"+buildVariant)
b.BuiltinArgDefaults["BUILDPLATFORM"] = cleanedBuildPlatform
b.BuiltinArgDefaults["BUILDVARIANT"] = ""
Copy link
Member

Choose a reason for hiding this comment

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

The "BUILD..." values appear to be set at https://github.com/openshift/imagebuilder/blob/master/dispatchers.go#L40-L56. Why not fix them there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if any other user of imagebuilder would expect that "BUILD..." should be empty by default to match docker.

Copy link
Member

Choose a reason for hiding this comment

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

Why would they expect something different?

Copy link
Member Author

Choose a reason for hiding this comment

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

They might expect something different because the current behavior (setting BUILD..). Changing it to empty by default would potentially break something that rely on it. But I will try to change it in imagebuilder.

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes getting of cpu variants for ARM64
Fixes the TestConformance/setuid-file-in-archive test on ARM64.

Signed-off-by: Jan Rodák <[email protected]>
New imagebuilder contains fix that sets empty TARGETVARIANT and BUILDVARIANT. Docker does not set these values for ARM64.
This fixes the TestConformance/multistage-builtin-args and TestConformance/builtins tests on ARM64.

Signed-off-by: Jan Rodák <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants