-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Use absolute path instead requestURI in openapiv3 discovery #117495
Use absolute path instead requestURI in openapiv3 discovery #117495
Conversation
82d6189
to
a9abcfc
Compare
/cc @alexzielenski @apelisse |
/triage accepted |
/priority important-soon |
Can we actually test this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is fine as it is, but I'd really like to see a test added.
Thanks for review @apelisse. I added unit tests to assure that patch fixes the issue. |
OK I think that's fine, thank you. We should ask someone else's opinion since my knowledge in that area is more limited. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
a few nits on the test case names and one duplicate test we don't need, lgtm otherwise
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
since we'll be cherry-picking this, I'd probably recommend squashing to a single commit here rather than having tide squash for you |
Currently, openapiv3 discovery uses requestURI to discover resources. However, that does not work when the rest endpoint contains prefixes (e.g. `http://localhost/test-endpoint/`). Because requestURI overwrites prefixes also in rest endpoint (e.g. `http://localhost/openapiv3/apis/apps/v1`). Since `absPath` keeps the prefixes in the rest endpoint, this PR changes to absPath instead requestURI.
f44faf6
to
80f42bb
Compare
/lgtm |
LGTM label has been added. Git tree hash: fad7d1e664821ee5762e1d6d23dd86008fbc79d8
|
Once this merges, can you open a cherry-pick to release-1.27 (and maybe 1.26 if applicable) |
…17495-upstream-release-1.27 Automated cherry pick of #117495: Use absolute path instead requestURI in openapiv3 discovery
…17495-upstream-release-1.26 Automated cherry pick of #117495: Use absolute path instead requestURI in openapiv3 discovery
…es#117495) Currently, openapiv3 discovery uses requestURI to discover resources. However, that does not work when the rest endpoint contains prefixes (e.g. `http://localhost/test-endpoint/`). Because requestURI overwrites prefixes also in rest endpoint (e.g. `http://localhost/openapiv3/apis/apps/v1`). Since `absPath` keeps the prefixes in the rest endpoint, this PR changes to absPath instead requestURI.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently, openapiv3 discovery uses requestURI to discover resources. However, that does not work when the rest endpoint contains prefixes (e.g.
http://localhost/test-endpoint/
).Because requestURI overwrites prefixes also in rest endpoint (e.g.
http://localhost/openapiv3/apis/apps/v1
).Since
absPath
keeps the prefixes in the rest endpoint, this PR changes to absPath instead requestURI(e.g.http://localhost/test-endpoint/openapiv3/apis/apps/v1
).Which issue(s) this PR fixes:
Fixes #117463
Does this PR introduce a user-facing change?