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

[issue #3661] http.send unix domain sockets #3667

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

kirk-patton
Copy link
Contributor

No description provided.

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 a lot for contributing! I've got a couple of nitpicks and a question re: http.Transport inline 👇 🙃

topdown/http.go Outdated Show resolved Hide resolved
topdown/http_test.go Outdated Show resolved Hide resolved
topdown/http_test.go Outdated Show resolved Hide resolved
topdown/http.go Outdated Show resolved Hide resolved
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 bearing with me. I'm afraid I've got a few more comments and questions 😅

topdown/http.go Outdated Show resolved Hide resolved
topdown/http_test.go Outdated Show resolved Hide resolved
topdown/http_test.go Outdated Show resolved Hide resolved
@srenatus
Copy link
Contributor

Sorry for the radio silence -- didn't manage to get back to this today. I'll take another look tomorrow.

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 bearing with me. I'm a little suspicious of the signal handler stuff -- do we really need that? That's the last bump, I think 🔍

Thanks again!

topdown/http_test.go Outdated Show resolved Hide resolved
topdown/http.go Show resolved Hide resolved
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 bearing with me. This looks good! A last nitpick, and I think we can get this merged 🎉

topdown/http_test.go Outdated Show resolved Hide resolved
srenatus
srenatus previously approved these changes Aug 11, 2021
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 contributing! One last thing (promised!) -- could you please squash your commits and a add a little commit message describing the change, please? There's some guidance in CONTRIBUTING.md.

@kirk-patton
Copy link
Contributor Author

ok, I think I got my commits squashed

@ashutosh-narkar
Copy link
Member

ashutosh-narkar commented Aug 11, 2021

@kirk-patton can you please update the http.send docs here and add a comment about the uds support.

Also you can squash your commits with something like git rebase -i HEAD~<number_of_your_commits> eg. git rebase -i HEAD~2

@kirk-patton
Copy link
Contributor Author

@kirk-patton can you please update the http.send docs here and add a comment about the uds support.

Also you can squash your commits with something like git rebase -i HEAD~<number_of_your_commits> eg. git rebase -i HEAD~2

@kirk-patton kirk-patton reopened this Aug 11, 2021
@kirk-patton
Copy link
Contributor Author

@kirk-patton can you please update the http.send docs here and add a comment about the uds support.

Also you can squash your commits with something like git rebase -i HEAD~<number_of_your_commits> eg. git rebase -i HEAD~2

Certainly 👍

@tsandall tsandall requested a review from srenatus August 12, 2021 20:20
@tsandall
Copy link
Member

@srenatus if this looks good, let's get this merged.

@kirk-patton
Copy link
Contributor Author


@srenatus if this looks good, let's get this merged.

while attempting to call the http.send() function using unix://localhost/?socket=%F2tmp%F2http.socket, I am getting undefined back. It does not appear that the code I added is getting called. I need to take closer look.

@kirk-patton
Copy link
Contributor Author

@srenatus if this looks good, let's get this merged.

while attempting to call the http.send() function using unix://localhost/?socket=%F2tmp%F2http.socket, I am getting undefined back. It does not appear that the code I added is getting called. I need to take closer look.

I double checked this morning and the cause of the issue I saw earlier was just that I needed to update my policy to use the new url format... It works

@srenatus
Copy link
Contributor

I'm afraid I had forgotten about it, but @ashutosh-narkar is correct, this could use a bit of documentation. Would you mind adding some?

@kirk-patton
Copy link
Contributor Author

I'm afraid I had forgotten about it, but @ashutosh-narkar is correct, this could use a bit of documentation. Would you mind adding some?

Yes, I will add that today. 👍

@ashutosh-narkar
Copy link
Member

@kirk-patton thanks for addressing our comments. Can you please squash your commits and sign-off your final commit. This link should help.

Something like:

$ git fetch upstream
$ git rebase upstream/main
$ git rebase -i HEAD~39               # you can squash your commits here and reword the commit message
$ git push -f ......                            # then force push changes to your fork

Let us know if any questions.

Signed-off-by: Kirk Patton <kpatton@verizonmedia.com>
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.

🎉 Thank you for contributing this, and thanks for bearing with us! 🚀 👏

@srenatus srenatus merged commit d047070 into open-policy-agent:main Aug 17, 2021
@srenatus
Copy link
Contributor

@kirk-patton the only thing that is a little off here is that github won't recognize the commit as yours -- I guess that's because the commit's email address hasn't been added to the list of addresses that are "yours" on github. https://docs.github.com/en/github/setting-up-and-managing-your-github-user-account/managing-email-preferences/setting-your-commit-email-address should have all the details, this is something that can be fixed retroactively if you don't mind having your yahoo email in github.

@kirk-patton kirk-patton deleted the socket branch August 17, 2021 20:12
jgchn pushed a commit to jgchn/opa that referenced this pull request Aug 20, 2021
Fixes open-policy-agent#3661.

Signed-off-by: Kirk Patton <kpatton@verizonmedia.com>
dolevf pushed a commit to dolevf/opa that referenced this pull request Nov 4, 2021
Fixes open-policy-agent#3661.

Signed-off-by: Kirk Patton <kpatton@verizonmedia.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
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