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

Add support for Go #129

Merged
merged 8 commits into from
May 30, 2023
Merged

Add support for Go #129

merged 8 commits into from
May 30, 2023

Conversation

mnafees
Copy link
Contributor

@mnafees mnafees commented May 16, 2023

This PR adds support for Go support.

Currently, only compiled WASM modules using TinyGo is supported. This comes as a result of TinyGo supporting WASI whereas the original Go compiler emits WASM that needs to be accompanied with a wrapper JS file.

To compile

tinygo build -o my_worker.wasm -target wasi my_worker.go

Resolves #95

@vmwclabot
Copy link

@mnafees, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@mnafees, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@Angelmmiguel Angelmmiguel added the 🚀 enhancement New feature or request label May 17, 2023
@vmwclabot
Copy link

@mnafees, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Can you please provide a full address ?

@vmwclabot
Copy link

@mnafees, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@mnafees mnafees marked this pull request as ready for review May 17, 2023 16:12
@vmwclabot
Copy link

@mnafees, VMware has approved your signed contributor license agreement.

@Angelmmiguel Angelmmiguel requested a review from a team May 18, 2023 06:44
@Angelmmiguel Angelmmiguel self-assigned this May 18, 2023
Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

Hello @mnafees ,

Thank you for this amazing contribution! I added two minor comments. The code looks great and all the examples work perfectly.

Apart from the examples, we should include a new documentation page for Go. It's not a blocker for merging this, so we can work on it in a separate PR. If it's fine for you, I would add a note in that documentation page mentioning you as the original author of the Go kit.

Thank you! 🎉

kits/go/worker/worker.go Outdated Show resolved Hide resolved
kits/go/worker/worker.go Outdated Show resolved Hide resolved
@mnafees
Copy link
Contributor Author

mnafees commented May 19, 2023

Hello @mnafees ,

Thank you for this amazing contribution! I added two minor comments. The code looks great and all the examples work perfectly.

Apart from the examples, we should include a new documentation page for Go. It's not a blocker for merging this, so we can work on it in a separate PR. If it's fine for you, I would add a note in that documentation page mentioning you as the original author of the Go kit.

Thank you! 🎉

I am going to add code comments as well as the docs for the new Go support kit and update the PR accordingly :)

@Angelmmiguel
Copy link
Contributor

Hey @mnafees!

This is just a check on the status of this PR :). I believe the current Go kit is in a great shape to included in the project.

If you are more busy lately, I can do a final review, merge it and create an issue to work on the documentation in a separate PR. If you prefer to complete the documentation on this PR, feel free to add it too.

No rush for it. I just wanted to to do a check since the latest comment was from a few days ago.

Thanks!

@mnafees
Copy link
Contributor Author

mnafees commented May 29, 2023

Hi @Angelmmiguel, apologies for being AWOL for a while. I got busy with other work. I am updating the PR with the docs. I also went ahead and chose to use the sjson library as suggested by you. Will update with a new comment once the PR is ready for your re-review. Thank you for the patience!

@Angelmmiguel
Copy link
Contributor

Hi @Angelmmiguel, apologies for being AWOL for a while. I got busy with other work. I am updating the PR with the docs. I also went ahead and chose to use the sjson library as suggested by you. Will update with a new comment once the PR is ready for your re-review. Thank you for the patience!

Hello @mnafees! No worries, you don't need to apologize 😄. I regularly check open PRs in case I can help on anything. Thank you very much for your contribution! 👏

@mnafees
Copy link
Contributor Author

mnafees commented May 30, 2023

@Angelmmiguel I have updated the PR with all of my changes to the documentation :)

@mnafees mnafees requested review from Angelmmiguel and ereslibre May 30, 2023 07:49
Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

The changes look great @mnafees! Thank you very much for this contribution 😄. I'm really happy to see that wws supports Go now 👏.

Btw, I'll write an article about a new release and I'll be sure to include the recent contributions like this one there 😄.

@Angelmmiguel
Copy link
Contributor

Btw, no worries about the issue on the CI. It's a flaky test I need to fix in main and it's not related to your changes. I will retry it after other builds finish.

@Angelmmiguel Angelmmiguel mentioned this pull request May 30, 2023
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Amazing contribution @mnafees! LGTM 👏, 🚢 it!

@Angelmmiguel Angelmmiguel merged commit 491e951 into vmware-labs:main May 30, 2023
@gzurl
Copy link
Contributor

gzurl commented Jun 15, 2023

Awesome contribution @mnafees! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Golang
5 participants