-
Notifications
You must be signed in to change notification settings - Fork 60
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
[RFR] Add support for apiPaths #869
Conversation
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.
👍
LGTM 👍 |
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, apiPath := range apiPaths { |
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 a reason not to turn this into a formatted list as you did above and then use that to iterate over?
Another question - should we take the opportunity here to make the apiPaths field require you to put the entire api path, not just the apiPath suffix?
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 just updated it to use the same approach and format the strings first.
Re the second question -- ok, yes we can do that here if you want to. I think I should just be able to use a +kubebuilder:validation:Pattern=<regex pattern>
annotation on the field, correct? What type of patterns do we want to permit here to start with? Only /api/<text>/
? Technically I think the regex for this will be something like:
//+kubuilder:validation:Pattern="^\/api\/[a-zA-Z-]+\/$"
Looks good to me, there is a failing test - I didn't look , but let me know if you need anything in helping debug it. |
/retest |
The assert in I'm not sure how my changes here would have impacted cert auth. We did modify |
/retest |
1 similar comment
/retest |
All checks passing now :) |
Thanks for this enhancement! |
Some apps have a need to expose two API paths to the same back-end. This adds support for a new
[]string
field calledapiPaths
-- in 'local' mode, Clowder will now configure theIngress
resource it creates with one or more paths.apiPath
will now become deprecated.