Skip to content
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

chore: shorten the route name for Ingress transformations #898

Merged
merged 1 commit into from Mar 6, 2022

Conversation

tao12345666333
Copy link
Member

Signed-off-by: Jintao Zhang zhangjintao9020@gmail.com

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

In the past, we used host + path directly to form its route name for readability,
but this method can cause problems in some scenarios.
For example, the generated name is too long.
The current APISIX limit its maximum length to 100.

We will construct the following structure for easy reading and debugging.
ing_namespace_ingressName_id

fix #781

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

Codecov Report

Merging #898 (a4dfc63) into master (5f6a7c1) will increase coverage by 0.06%.
The diff coverage is 84.00%.

❗ Current head a4dfc63 differs from pull request most recent head 47e9e22. Consider uploading reports for the commit 47e9e22 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #898      +/-   ##
==========================================
+ Coverage   32.01%   32.08%   +0.06%     
==========================================
  Files          71       72       +1     
  Lines        7764     7771       +7     
==========================================
+ Hits         2486     2493       +7     
  Misses       5004     5004              
  Partials      274      274              
Impacted Files Coverage Δ
pkg/ingress/pod.go 37.34% <71.42%> (+2.34%) ⬆️
pkg/kube/translation/ingress.go 77.81% <100.00%> (+0.22%) ⬆️
test/e2e/e2e.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f6a7c1...47e9e22. Read the comment docs.

@tao12345666333
Copy link
Member Author

@gxthrj PTAL

@tao12345666333
Copy link
Member Author

ping @gxthrj

@gxthrj gxthrj merged commit 8348d01 into apache:master Mar 6, 2022
@tao12345666333 tao12345666333 added this to In progress in v1.5 Planning via automation Mar 6, 2022
@tao12345666333 tao12345666333 added this to the 1.5.0 milestone Mar 6, 2022
tao12345666333 added a commit to tao12345666333/ingress-controller that referenced this pull request Apr 22, 2022
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333 tao12345666333 deleted the fix-route-name branch April 22, 2022 09:43
@tao12345666333 tao12345666333 modified the milestones: 1.5.0, 1.4.1 Apr 22, 2022
@tao12345666333 tao12345666333 moved this from In progress to Done in v1.5 Planning Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

request help: The route name generated by default is too long
4 participants