-
Notifications
You must be signed in to change notification settings - Fork 33
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
Authpolicy/v1beta3 #924
base: main
Are you sure you want to change the base?
Authpolicy/v1beta3 #924
Conversation
2860c69
to
8517495
Compare
First pass at adding v1beta3. This is not expected to compile. Signed-off-by: Jim Fitzpatrick <[email protected]>
Signed-off-by: Jim Fitzpatrick <[email protected]>
Signed-off-by: Jim Fitzpatrick <[email protected]>
Remove the reference to the RouteSelector from the reference docs. Signed-off-by: Jim Fitzpatrick <[email protected]>
Signed-off-by: Jim Fitzpatrick <[email protected]>
8517495
to
14e4762
Compare
Signed-off-by: Jim Fitzpatrick <[email protected]>
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.
Can we also delete api/v1beta2/route_selectors.go
?
@@ -1,4 +1,4 @@ | |||
package v1beta2 | |||
package v1beta3 |
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 we can also remove all +kubebuilder:validation:MaxProperties=10
markers. They were added to keep the CRD within the complexity budget when we first introduced reoute selectors.
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.
Yap can update this. I expect this will should also effect some tests also.
// GetRulesHostnames | ||
// in v1beta2 this returned the list of route selectors | ||
// in v1beta3 this should work with section name, once implemented. | ||
func (ap *AuthPolicy) GetRulesHostnames() []string { |
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 there anything left that still depends on this? Maybe it's required to fully implement some interface by the actual method is no longer being used? (I didn't check.)
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.
Need to dig a bit more into this one.
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.
This needs to be there to implement an interface
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 generally but I think there's some changes missing 🧑💻
Can we also update the api version of the guides where an AuthPolicy
is used as it's now using the wrong api version?
Can we rename and update the authpolicy v1beta2 sample to v1beta3 so that the example is still shown in the generated csv?
@KevFan The sample authpolicy that I can update no problem. I don't feel updating the guides right now is the best thing to do right now. These guides will need to be again updated when the section name is introduced. I had the intentional decision not to update guides till we had the section name in place. |
@guicassolato yes, I had it remove and reintroduced it when trying to fix test. I most have forgotten to remove it afterwards. It was never meant to make it into this PR. |
Signed-off-by: Jim Fitzpatrick <[email protected]>
f82699a
to
044e52b
Compare
Closes: #809
This change introduces the AuthPolicy v1beta3.
The RouteSelector has being removed from the API. There is still work required to bring in the Section Name to the API.
In the Docs only the reference to the RouteSelector has being removed from the reference sections. The guides have not being updated to use v1betat3. I expect this to happen with or shortly after the adding of the Section Name.
A number of integration test have been removed which depend on or directly tested the behavior around the RouteSelector.