Remove TrueField type in CORS #3895
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
kind/gep
shaneutt
added
ok-to-test
shaneutt
added
the
v1.4-release/subtask
k8s-ci-robot
added
the
size/L
apis/v1/httproute_types.go
Outdated
| // 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.
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.
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 |
shaneutt
added
do-not-merge/hold
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Might want to make the docs clear that omitted and false are the same
apis/v1/httproute_types.go
Outdated
| // When set to false the gateway will omit the header | ||
| // `Access-Control-Allow-Credentials` entirely (this is the standard CORS | ||
| // behavior). |
k8s-ci-robot
removed
the
lgtm
shaneutt
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
do-not-merge/hold
|
/lgtm Thanks @shaneutt! I went ahead and left a suggestion about explicitly setting to |
k8s-ci-robot
added
the
lgtm
| // | ||
| // +optional | ||
| AllowCredentials TrueField `json:"allowCredentials,omitempty"` | ||
| AllowCredentials *bool `json:"allowCredentials,omitempty"` |
There was a problem hiding this comment.
Did the actual TrueField type get removed in this PR somewhere? I can't seem to find it.
Related question - I'd assume we only want a pointer here if we want to distinguish between the unspecified and zero value here, but I don't think we do here.
There was a problem hiding this comment.
Did the actual TrueField type get removed in this PR somewhere? I can't seem to find it.
👍 2f11de8
Related question - I'd assume we only want a pointer here if we want to distinguish between the unspecified and zero value here, but I don't think we do here.
I was deliberating about this, however note that the API Conventions were recently updated around this specifically:
For `bool` types, the zero value is `false`, which is always a valid user choice. `bool` types should always be pointers, and should always use the `omitempty` tag.
Signed-off-by: Shane Utt <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
shaneutt
force-pushed
the
cors-allowed-bool
branch
from
02b7959 to
45a3424
Compare
shaneutt
removed
the
do-not-merge/hold
|
🚢 Ship it! 🙂 /lgtm |
k8s-ci-robot
added
the
lgtm
8 participants
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?: