-
Notifications
You must be signed in to change notification settings - Fork 343
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
feat: add externalname support for upstream #726
Conversation
@elvis-cai Please add some test cases to cover these changes. |
sure, doc & test added. |
Codecov Report
@@ Coverage Diff @@
## master #726 +/- ##
==========================================
- Coverage 32.50% 32.37% -0.13%
==========================================
Files 66 66
Lines 6800 6836 +36
==========================================
+ Hits 2210 2213 +3
- Misses 4340 4362 +22
- Partials 250 261 +11
Continue to review full report at Codecov.
|
metadata: | ||
name: foo-service | ||
spec: | ||
httpHostRewritePolicy: |
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.
The specification is bad. You should split them.
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.
@elvis-cai and this one
@@ -23,6 +23,15 @@ import ( | |||
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2alpha1" | |||
) | |||
|
|||
const ( | |||
// UseOriginalClientHost means host option set to "pass" for the upstream | |||
UseOriginalClientHost = "useOriginalClientHost" |
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.
As per the Kubernetes convention, the value should be in upper camel case. Like values of ImagePullPolicy
: Always
, Never
, IfNotPresent
.
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.
Please fix the CI errors.
my bad, last commit forget to update test code, updated |
Sorry for delay. Can you resolve the conflict? Then we run the test and merge this PR. Thanks! |
all good, conflict resolved and feel free to rerun the test, thanks. |
@tokers @tao12345666333 Why this feature not merge to master, Can you go ahead to process this feature ? |
There are too many conflicts to resolve. |
@elvis-cai Could you merge the newest master branch? Thanks! |
This looks like a great feature and I'm really looking forward to having it merged. Is there something I can do to help move it along? |
I am more looking forward to the feature #927, but sounds good to me , before that's implemented, will look at if I could fix the conflict and merge the latest master branch. |
12563f5
to
9b4627b
Compare
@tao12345666333 how does this look? I'd love to see this PR merged |
Just adding my +1 for this feature :) |
Sorry for delay reply. #726 #927 and some related tasks, I will put them in the v1.6 version to complete. Currently we have too many things to do, including GitHub issues, PRs, some features and bugfixes, etc., and Slack channel replies. This leaves us almost at full capacity. If anyone would like to see some new features added, please join us and help us make this project better, thanks |
#1306 has been merged. I will close this one, thanks! |
@tao12345666333 It looks like there's a discrepancy in the docs and what's actually possible when using the I'm not sure where the confusion happened but it looks like this pr would have added that feature, but #1306 was merged instead and does include |
Please answer these questions before submitting a pull request
Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
request help: configure backend service with external url. #645
Bugfix
Description
How to fix?
New feature or improvement
ExternalName
supportpassHost
andupstreamHost
Let me know if there's anything missing here, thanks.
Backport patches
Why need to backport?
Source branch
Related commits and pull requests
Target branch