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

Make it possible to only build wasm testcases #6920

Merged

Conversation

andreaTP
Copy link
Contributor

@andreaTP andreaTP commented Aug 8, 2024

Why the changes in this PR are needed?

Tackling this "TODO":
https://github.com/open-policy-agent/npm-opa-wasm/blob/92b8aa0234a7e487f2bd335ea1544351718972f7/.github/workflows/ci.yml#L50-L52

as I'm in the same situation:
https://github.com/andreaTP/opa-chicory/blob/d37da558e054510e50da53bf7a1933e0bc19a136/.github/workflows/ci.yml#L33-L34

What are the changes in this PR?

Add the handling for an env var WASM_BUILD_ONLY so that, running:

WASM_BUILD_ONLY=true make wasm-rego-test

Would not run the tests saving time and container images downloads.

Notes to assist PR review:

Attempted to do the least intrusive possible change.

Further comments:

An additional step would be to upload the resulting testcases.tar.gz to GH releases, but I'm looking for feedback before doing the changes.

Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 7670e4f
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66b4952ae373df0009f440d2
😎 Deploy Preview https://deploy-preview-6920--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@anderseknert
Copy link
Member

Hi there, and thanks!

Should you not remove the TODO comment if you resolve that by this fix? :)

The change looks reasonable to me, but I'll let someone else have a say too.

You'll need to sign off on your commit though for the DCO check. You can just do:

git commit --amend --signoff
git push --force

To fix that.

Signed-off-by: Andrea Peruffo <andrea.peruffo1982@gmail.com>
@andreaTP
Copy link
Contributor Author

andreaTP commented Aug 8, 2024

Thanks for the review @anderseknert !
The DCO should be fixed now, regarding the TODO, it will require to be merged and used in the SDKs repos to be removed, this PR is just "enabling" a fix down the line.

@anderseknert
Copy link
Member

Ah, sorry, I didn't notice that the TODO was in another repo! All good then 👍

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!!

@@ -16,13 +16,18 @@ ASSETS=${ASSETS:-"$PWD/test/wasm/assets"}
VERBOSE=${VERBOSE:-"0"}
TESTGEN_CONTAINER_NAME="opa-wasm-testgen-container"
TESTRUN_CONTAINER_NAME="opa-wasm-testrun-container"
WASM_BUILD_ONLY=${WASM_BUILD_ONLY:-"false"}
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I think if we're checking for != "true" below, leaving the default unset would be OK. But it doesn't matter, really.

@ashutosh-narkar ashutosh-narkar merged commit 83f8dc6 into open-policy-agent:main Aug 14, 2024
28 checks passed
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.

4 participants