search

proxy-wasm / test-framework

WebAssembly for Proxies (test framework)
Apache License 2.0
16 stars 13 forks source link

Catch up on the world #12

Closed alexsnaps closed 1 year ago

alexsnaps commented 1 year ago

This uses wasmtime ~9.0.3~ 11.0.0 (latest). I needed this to run on aarch. This led to more upgrades in order to get the tests to pass:

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

alexsnaps commented 1 year ago

Is there any interests in getting this up to date, or not really? Slowly getting this to be usable with our wasm module, but wondering if there is any interest in such PRs moving forward, especially if other things I don't know of should be accounted for... thanks!

PiotrSikora commented 1 year ago

@alexsnaps Definitely! But note that you'd be doing most (all?) of the work, since this project is currently unmaintained. Also, I'm biased, since this project was my idea...

cc @mpwarres @martijneken to get Google's take on this.

alexsnaps commented 1 year ago

But note that you'd be doing most (all?) of the work, since this project is currently unmaintained.

Well, not doing all of the work right now, but still getting this somewhat further along to support our current use-case as we are looking at putting more responsibilities within that. So there is always that :)

@mpwarres @martijneken to get Google's take on this.

I, and probably others on the team, would be willing to help move this forward. So if it makes sense, or if there is more/different aspects that you'd care about, let me know (e.g. that license check...).

And if it doesn't, we'd probably depend on a fork of this with our changes in, but thinking this could benefit others.

PiotrSikora commented 1 year ago

To make sure this isn't deadlocked - are you willing to fix licenses and examples tests or are you waiting for me to fix them?

alexsnaps commented 1 year ago

@PiotrSikora Surely wasn't expecting you to fix it, no. And I can take time to address these... it still wasn't clear to me whether there was interest in this PR.

On the license check, I'm sure that's trivial. On the examples tho, fairly sure those would break because of a signature mismatch. But I fixed that (along other work, eg gRPC calls) on some other branch.

Anyways, I'm traveling but will squeeze this in in the coming week then.

PiotrSikora commented 1 year ago

On the license check, I'm sure that's trivial.

It is, see: https://github.com/proxy-wasm/proxy-wasm-rust-sdk/pull/118.

On the examples tho, fairly sure those would break because of a signature mismatch. But I fixed that (along other work, eg gRPC calls) on some other branch.

They're broken because of a reorg of examples to separate crates in https://github.com/proxy-wasm/proxy-wasm-rust-sdk/pull/146.

Anyways, I'm traveling but will squeeze this in in the coming week then.

Perfect, thanks! No rush, it's been abandoned for a few years, so a week or two doesn't matter.

alexsnaps commented 1 year ago

I think this should now be a green build. Let me know what you think. Thanks!

alexsnaps commented 1 year ago

I think this should now be a green build. Let me know what you think. Thanks!

And it did 🙌 - Let me know if all looks good to you still. I would open a follow up PR with gRPC (and possibly a few other additions) which represents the work it took to get our module tested. Thanks!

alexsnaps commented 1 year ago

Would it be possible to split them into separate PRs?

Do you mean PRs? Or commits? Cause I cannot make separate PRs that will pass the tests:

So this is a bit of an egg or chicken problem. The commits are somewhat individual changes, but I could group them in those 4 "tasks" in this one PR.

alexsnaps commented 1 year ago

I re-arranged the commits :

Let me know what you think. That last items requires fixes to the tests themselves as well as ABI changes. I guess I'd merge this PR, but without squashing.

PiotrSikora commented 1 year ago

Do you mean PRs? Or commits? Cause I cannot make separate PRs that will pass the tests:

  • cargo outdated will fail w/o the latest wasm-time
  • the wasm-time will not work with the code as is
  • the examples are compiled targeting a newer ABI
  • can't have these pass the tests without support for 0.2.1

So this is a bit of an egg or chicken problem. The commits are somewhat individual changes, but I could group them in those 4 "tasks" in this one PR.

I guess I'd merge this PR, but without squashing.

Well, that's the reason behind my ask to split this into separate PRs. The CI is broken, so merging individual PRs that don't pass is fine as long as we know that the whole chain is passing them. If you split this, then we can merge them one by one.

Let me know what you think. That last items requires fixes to the tests themselves as well as ABI changes.

Thanks! This looks great.

alexsnaps commented 1 year ago

Well, that's the reason behind my ask to split this into separate PRs. The CI is broken, so merging individual PRs that don't pass is fine as long as we know that the whole chain is passing them.

This PR here would be the first step then, split in two commits. Then:

Does that make sense @PiotrSikora ?

PiotrSikora commented 1 year ago

Well, that's the reason behind my ask to split this into separate PRs. The CI is broken, so merging individual PRs that don't pass is fine as long as we know that the whole chain is passing them.

This PR here would be the first step then, split in two commits. Then:

  • [ ] Fix cargo outdated with this commit, which should fix the CI
  • [ ] Finally, fix running the latest examples and make the test pass (this last step could be 2 commits, fix the tests themselves, and add minimal support for the latest ABI to run these tests)

Does that make sense @PiotrSikora ?

It does (sans the PR with two commits - in general PRs get squashed into one, but see comments in that PR).

Also, sorry for the delay and thanks for the patience!