-
Notifications
You must be signed in to change notification settings - Fork 555
Remove TrueField type in CORS #3895
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
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
// When set to false the gateway will omit the header | ||
// `Access-Control-Allow-Credentials` entirely (this is the standard CORS | ||
// behavior). |
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.
While in the previous versions of this feature we actually only provided true
, here we can also provide false
, so i'm thinking that maybe we'll need to add a test that makes sure that setting it to false actually omits it (in the gateway implementation itself), just out of thought that maybe existing implementations take the value (if exists), that in the past could have only been true
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.
Yeah, sounds good. I am in no position at the moment to test an implementation with CORS support in order to develop this however, so we should throw up a follow-up issue to track additional testing needed for CORS and link that as a sub-task of #1767. Since you seem to be very clear on it, would you mind creating that issue and linking it here, and I can link it as a sub-task?
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
883e087
to
a162726
Compare
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.
Looks good to me, holding for other LGTMs though.
/lgtm
/hold
geps/gep-1767/index.md
Outdated
type: CORS | ||
``` | ||
|
||
In this case, the gateway will not include the |
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.
Is it worth discussing how this would be mostly useful as a way to override a default? or not so much?
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.
Would you mind writing up a Github suggestion here, to better detail what you're thinking?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kflynn, shaneutt 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 |
New changes are detected. LGTM label has been removed. |
67ef279
to
77dcf18
Compare
Signed-off-by: Shane Utt <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
77dcf18
to
0cc5643
Compare
What type of PR is this?
/kind feature
/kind bug
Which issue(s) this PR fixes:
Implements the fix discuseed in #3841, and fixes #3841.
Does this PR introduce a user-facing change?: