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

feat: adds tinygo support. #254

Merged
merged 42 commits into from
Aug 16, 2022
Merged

Conversation

jcchavezs
Copy link
Member

@jcchavezs jcchavezs commented May 27, 2022

This PR adds proper support for tinygo.

HTTP server example has been turned into an own module to avoid those files to be considered in the tinygo test.

Depends on #296

@jcchavezs jcchavezs force-pushed the tinygo_support branch 4 times, most recently from 9b6a9bf to 66531fd Compare May 30, 2022 21:29
@jcchavezs jcchavezs force-pushed the tinygo_support branch 2 times, most recently from 5d2fdce to 9d6f6cf Compare June 22, 2022 22:22
@jcchavezs jcchavezs changed the base branch from v2/master to v3/dev July 8, 2022 09:10
@jcchavezs
Copy link
Member Author

I need to get this sorted first #283

@jcchavezs
Copy link
Member Author

Once #296 is merged we can get more greens.


- name: Tests
run: go list ./... | xargs -I{} tinygo test {}
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 is the main goal @anuraaga

@anuraaga
Copy link
Contributor

anuraaga commented Aug 4, 2022

@jptosso - I think we're almost ready with this. I have split out #307, would it help to further split out parts of this PR? Also, what do you think about converting the JSON test harnesses to Go, similar to #306? While tinyjson can generate code that works, perhaps it's unnecessary anyways.

@anuraaga
Copy link
Contributor

anuraaga commented Aug 8, 2022

Answering my own question, I guess it is probably not a good idea to convert the JSON to go. I found what seems to be the original source for the JSON files so we wouldn't want to diverge too much I think.

No problem, actually instead of generating some large tinyjson files, I'll rework to use gjson after #311.

@anuraaga
Copy link
Contributor

@jptosso @jcchavezs Thanks for the patience, I think this is ready for a final review / merge

@anuraaga
Copy link
Contributor

@jptosso Merged upstream and add some remaining fixes. Would be great if you can take a look at this as it'll be a significant milestone to Envoy support

Copy link
Member

@jptosso jptosso left a comment

Choose a reason for hiding this comment

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

https://github.com/corazawaf/coraza/pull/254/files#diff-fbcd29a8693c2ec9956d0422d695d516d8a465ecf998ab4c2f9d17863a65556dL310 could be replaced with type assertion. CRS tests uses this feature. Everything else looks good to me.

Great work!

@@ -61,7 +61,7 @@ type Profile struct {

type ExpectedOutput struct {
Headers map[string]string `yaml:"headers,omitempty"`
Data interface{} `yaml:"data,omitempty"` // Accepts array or string
Copy link
Member

Choose a reason for hiding this comment

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

CRS tests will use an array of lines or a string, we must validate if the tests has been refactored or we can send a PR

testing/engine.go Show resolved Hide resolved
seclang/rule_parser_test.go Show resolved Hide resolved
@jptosso jptosso enabled auto-merge (squash) August 16, 2022 04:10
auto-merge was automatically disabled August 16, 2022 05:09

Head branch was pushed to by a user without write access

@anuraaga
Copy link
Contributor

Thanks @jptosso! Fixed the issue.

By the way, if you intended for auto-merge to work, not yet unfortunately

Screen Shot 2022-08-16 at 14 10 34

Copy link
Member

@jptosso jptosso left a comment

Choose a reason for hiding this comment

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

LGTM

@jptosso jptosso merged commit a867605 into corazawaf:v3/dev Aug 16, 2022
@jptosso
Copy link
Member

jptosso commented Aug 16, 2022

@anuraaga @jcchavezs do we have regression tests for tinygo to avoid breaking this milestone?

@jcchavezs jcchavezs deleted the tinygo_support branch August 18, 2022 08:16
@jcchavezs
Copy link
Member Author

This is amazing milestone. Indeed we have the tinygo test command running on this which avoids regressions. Thanks @anuraaga

@jcchavezs
Copy link
Member Author

jcchavezs commented Oct 11, 2022 via email

@jcchavezs
Copy link
Member Author

jcchavezs commented Oct 11, 2022 via email

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