Skip to content

MAISTRA-2648: WASM fix for s390x - #130

Closed
knm3000 wants to merge 1 commit into
maistra:maistra-2.1from
knm3000:wasm-on-z
Closed

MAISTRA-2648: WASM fix for s390x#130
knm3000 wants to merge 1 commit into
maistra:maistra-2.1from
knm3000:wasm-on-z

Conversation

@knm3000

@knm3000 knm3000 commented Feb 10, 2022

Copy link
Copy Markdown

Fixes https://issues.redhat.com/browse/MAISTRA-2648
The permanent fix is not a part of maistra/envoy yet:
proxy-wasm/proxy-wasm-cpp-host#198

@maistra-bot

Copy link
Copy Markdown

Hi @knm3000. Thanks for your PR.

I'm waiting for a maistra member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jwendell

Copy link
Copy Markdown
Member

/ok-to-test

@jwendell jwendell changed the title WASM fix for s390x MAISTRA-2648: WASM fix for s390x Feb 16, 2022

@jwendell jwendell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@oschaaf oschaaf left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a thought about the way the required changes get patched in, the patch itself LGTM

Comment thread maistra/ci/common.sh

# wasm fix for s390x https://issues.redhat.com/browse/MAISTRA-2648
# the permanent fix is https://github.com/proxy-wasm/proxy-wasm-cpp-host/pull/198 (not a part of maistra/envoy yet)
patch -p1 -i maistra/ci/proxy-wasm-cpp-host.patch No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bazel provides a way to patch in changes to dependencies as well; which might be a little cleaner?
We could set up _proxy_wasm_cpp_host to apply the patch over at
https://github.com/maistra/envoy/blob/0f592ba9b37a354a97da929722f0c70d7d85106e/bazel/repositories.bzl#L793
WDYT?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(following up on this would imply moving this PR over to maistra/envoy)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review, I'll try to move the patch to maistra/envoy (and then update https://github.com/maistra/proxy/blob/maistra-2.1/WORKSPACE with the new envoy commit).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not necessary to touch proxy after this. After your PR is merged on envoy side, automation takes place and will open a PR in proxy with the changes that got merged in envoy.

@knm3000

knm3000 commented Feb 22, 2022

Copy link
Copy Markdown
Author

Opened maistra/envoy PR maistra/envoy#134 instead of this one.

@knm3000 knm3000 closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants