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

Update node_proxy.lua - Fix for interactive apps not working #3791

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

giuliolibrando
Copy link
Contributor

Fix for interactive apps not working after Apache 2.4.62, specifically when arguments are passed with ? character

completed the fix of the issue OSC#3730
Fix for interactive apps not working after Apache 2.4.62, specifically when arguments are passed with ? character
@johrstrom
Copy link
Contributor

E2E tests are failing, indicating this change does not do what we'd expect.

@johrstrom
Copy link
Contributor

Thanks for the contribution(s), but as I say, this breaks end to end tests, so I can't merge it as is.

@johrstrom
Copy link
Contributor

I'm looking into this now to get it backported to 3.1. What's the status of it on your side @giuliolibrando? I'm working on more end to end tests to verify the behaviors, and may have a patch that works.

Should I just commit to this pull request or should I close this and open another?

@giuliolibrando
Copy link
Contributor Author

I haven't had a chance to do more tests to understand why the merge pipeline tests are failing, but the apps are working correctly on my local installation. If you've made a better fix, feel free to commit it to this PR. Or let me know if I can do something or do tests

@johrstrom
Copy link
Contributor

It's hard for me to commit to your branch on master. In any case, I applied my fix to #3827.

As to the tests that are passing - you can do this to check: Rstudio's help menus send relative request. i.e., ../../some/help/html/docs. If you tested this against Rstudio it may work for the most part - but I'd guess that the documentation panel is broken.

@giuliolibrando
Copy link
Contributor Author

giuliolibrando commented Sep 27, 2024

@johrstrom if you're referring to this panel seems to work correctly. I tried to navigate some pages and they all loads
image

@johrstrom
Copy link
Contributor

@johrstrom if you're referring to this panel seems to work correctly. I tried to navigate some pages and they all loads

Yes it's in that panel. Well Rstudio used to return relative links, I'd have to inspect it if the newer versions do.

I don't know for sure if they still do or if package help text like this is the same URL construction as the help page you've shown. It's in the same panel, but help text for packages was what was broken and what these tests test for, but again, I'm not even sure if RStudio is returning relative URLs like they used to.

image

@johrstrom
Copy link
Contributor

In any case - I've patched the lua file in this pull request to what I believe works (and have written tests for). Can you please confirm? (and sorry for the delay on this, it's been a bit of a nightmare since httpd updated and broke this)

@robinkar
Copy link
Contributor

robinkar commented Oct 2, 2024

I thought I'd let you know that this fix seems to work for us at least with Rocky 8.10.
VSCode, for example, does not work in 3.1.8, but works after applying these changes.

@johrstrom johrstrom merged commit 7e2ab8b into OSC:master Oct 3, 2024
23 checks passed
johrstrom added a commit that referenced this pull request Oct 7, 2024
Fix node_proxy.lua for more issues related to #3730. Specifically #3825 where URIs with query parameters are not being passed correctly.

---------

Co-authored-by: Jeff Ohrstrom <[email protected]>
johrstrom added a commit that referenced this pull request Oct 7, 2024
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.

4 participants