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

Feature/connect improvements #254

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

GuyLewin
Copy link
Contributor

Solving #252 and #251

@GuyLewin
Copy link
Contributor Author

@pintsized let me know if there are any adjustments necessary

@GuyLewin
Copy link
Contributor Author

GuyLewin commented Oct 4, 2021

@pintsized what do you think?

@pintsized
Copy link
Member

Looks good, although we'll need to update the README and add some tests. Are you able to include them in this PR? I can help with the tests if you're not familiar with it? I don't think we really have proper coverage for return values from connect to be honest, so would be great to have.

@GuyLewin GuyLewin force-pushed the feature/connect-improvements branch from 1e8a8f4 to d12d6a9 Compare October 8, 2021 23:06
@GuyLewin
Copy link
Contributor Author

GuyLewin commented Oct 8, 2021

@pintsized I tried adding UTs but I didn't have a good template to setup a local SSL server + use lua-resty-http as a client. If you can come up with a sample test for that - I can enhance and add more UTs.
I tried using www.google.com as the SSL server but that didn't work as well through GitHub Actions (the tests passed locally though). You can see all the logs in my branch - https://github.com/GuyLewin/lua-resty-http/actions/runs/1322151032
In any case - I updated the README. Hope we can proceed with the tests separately - but let me know if there's anything else for me to do!

@GuyLewin
Copy link
Contributor Author

@pintsized once we merge this, I'll create a separate PR to fix #253 . I'm trying to avoid merge conflicts by doing one at a time. Do you think you'll have time to review this soon?

@pintsized
Copy link
Member

@GuyLewin ok great, sounds good. And thank you! Yes, I promise I'll get to it soon, I'm just pretty busy atm. I'll see if I can get some SSL tests going on this though, so leave that to me.

@GuyLewin
Copy link
Contributor Author

@pintsized sounds great, thanks! Let me know if there's any further changes needed in this repo. Otherwise I'll start working on the next one.

@GuyLewin
Copy link
Contributor Author

@pintsized until this is approved, do you mind adding the hacktoberfest-accepted label to this PR and the other ones I opened?
It'll help me achieve the minimum threshold for Hacktoberfest :)
https://hacktoberfest.digitalocean.com/resources/participation

@pintsized
Copy link
Member

@pintsized until this is approved, do you mind adding the hacktoberfest-accepted label to this PR and the other ones I opened?
It'll help me achieve the minimum threshold for Hacktoberfest :)

Sure thing! Sorry again, I am getting there, just snowed under. Thanks for the contributions!

@GuyLewin
Copy link
Contributor Author

@pintsized until this is approved, do you mind adding the hacktoberfest-accepted label to this PR and the other ones I opened?
It'll help me achieve the minimum threshold for Hacktoberfest :)

Sure thing! Sorry again, I am getting there, just snowed under. Thanks for the contributions!

Thanks! No worries, we forked these branches into our codebase for now until we merge it into the upstream.

I'll also try closing and re-opening this PR in particular since I created it before Hacktoberfest.

@GuyLewin GuyLewin closed this Oct 25, 2021
@GuyLewin GuyLewin reopened this Oct 25, 2021
@pintsized pintsized merged commit 26dfd76 into ledgetech:master Nov 22, 2021
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.

2 participants