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

feat: support ingress v1beta1 https #596

Merged
merged 9 commits into from
Aug 4, 2021

Conversation

tianshimoyi
Copy link
Contributor

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.

}

func (m *manifest) diff(om *manifest) (added, updated, deleted *manifest) {
// TODO add diff ssl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the TODO.

@@ -93,7 +96,37 @@ func (t *translator) translateIngressV1beta1(ing *networkingv1beta1.Ingress) (*T
upstreamMap: make(map[string]struct{}),
}
plugins := t.translateAnnotations(ing.Annotations)

// TODO add https
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@gxthrj
Copy link
Contributor

gxthrj commented Jul 15, 2021

@tao12345666333 tao12345666333 self-assigned this Jul 15, 2021
@tao12345666333 tao12345666333 added changelog Issues with this label should be added to changelog when public a new release feature labels Jul 15, 2021
@@ -168,6 +170,7 @@ func (c *ingressController) sync(ctx context.Context, ev *types.Event) error {
om := &manifest{
routes: oldCtx.Routes,
upstreams: oldCtx.Upstreams,
ssl: oldCtx.SSL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style.

@tokers
Copy link
Contributor

tokers commented Jul 16, 2021

@tianshimoyi Please check out the CI problems.

@70data
Copy link

70data commented Jul 16, 2021

@tianshimoyi Please format the code, then add UT and e2e test.

pkg/ingress/manifest.go Outdated Show resolved Hide resolved
test/e2e/ingress/ingress.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Merging #596 (54d48ef) into master (7291212) will decrease coverage by 0.25%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   34.40%   34.14%   -0.26%     
==========================================
  Files          55       55              
  Lines        5468     5530      +62     
==========================================
+ Hits         1881     1888       +7     
- Misses       3362     3416      +54     
- Partials      225      226       +1     
Impacted Files Coverage Δ
pkg/ingress/ingress.go 8.29% <0.00%> (-0.12%) ⬇️
pkg/kube/translation/context.go 85.71% <0.00%> (-14.29%) ⬇️
pkg/kube/translation/ingress.go 77.48% <0.00%> (-11.14%) ⬇️
pkg/ingress/manifest.go 53.74% <27.77%> (-9.42%) ⬇️

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 7291212...54d48ef. Read the comment docs.

@tokers
Copy link
Contributor

tokers commented Jul 20, 2021

Look good to me.

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check the lint error (format code)

@tianshimoyi
Copy link
Contributor Author

please check the lint error (format code)

The reason is that I didn't download the dependency under test / exe, which has been solved.

@tianshimoyi
Copy link
Contributor Author

[Fail] ApisixRoute Testing [It] create, update, remove k8s service, remove ApisixRoute

What is the cause of this error? I did not modify this place.

@tokers
Copy link
Contributor

tokers commented Jul 21, 2021

[Fail] ApisixRoute Testing [It] create, update, remove k8s service, remove ApisixRoute

What is the cause of this error? I did not modify this place.

Just re-run it.

@tianshimoyi
Copy link
Contributor Author

[Fail] ApisixRoute Testing [It] create, update, remove k8s service, remove ApisixRoute
What is the cause of this error? I did not modify this place.

Just re-run it.

Looks more wrong

截屏2021-07-21 上午10 55 19

@tao12345666333
Copy link
Member

re-run e2e jobs.

@tao12345666333 tao12345666333 added this to the 1.2.0 milestone Jul 22, 2021
@tianshimoyi
Copy link
Contributor Author

This error has appeared many times, this error is not my error, please correct it.
截屏2021-07-22 下午2 02 27

@tao12345666333
Copy link
Member

thanks for your contributions, I will check it

@@ -15,19 +15,20 @@
package translation

import (
"go.uber.org/zap"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please classify the import packages.

This is a 3rd party packages, and it should be put with other 3rd party packages.

@tao12345666333 tao12345666333 changed the title support ingress v1beta1 https feat: support ingress v1beta1 https Aug 4, 2021
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tao12345666333 tao12345666333 merged commit ac25764 into apache:master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issues with this label should be added to changelog when public a new release feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants