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

Adds rewrite-target annotation, tests for Nginx Annotations… #637

Merged
merged 2 commits into from
Jun 17, 2020
Merged

Adds rewrite-target annotation, tests for Nginx Annotations… #637

merged 2 commits into from
Jun 17, 2020

Conversation

retpolanne
Copy link
Contributor

This PR adds the nginx.ingress.kubernetes.io/rewrite-target: / when ingress class is nginx, adds tests for Nginx Annotations and attempts to create a singleton for building a kubernetes fake client.

Fixes #636

@@ -374,7 +374,31 @@ func TestHasAnotherIngress(t *testing.T) {
t.Fatal("got unexpected error:", err)
}

if rs != false {
if rs != true {
t.Errorf("got true; want false")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original test was wrong 😅 . We create another ingress, so HasAnotherIngress should've returned true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returned false because the k8s state wasn't persisted between function calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... Maybe we could remove the test or change error messgage

Copy link
Contributor

Choose a reason for hiding this comment

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

☝️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drgarcia1986 I'll change the error message and the variable names, so it'll become less confusing.

pkg/server/k8s/client_test.go Outdated Show resolved Hide resolved
@retpolanne retpolanne marked this pull request as ready for review June 17, 2020 14:02
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2020

Codecov Report

Merging #637 into master will decrease coverage by 0.22%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #637      +/-   ##
==========================================
- Coverage   50.48%   50.25%   -0.23%     
==========================================
  Files          86       86              
  Lines        6224     6230       +6     
==========================================
- Hits         3142     3131      -11     
- Misses       2867     2887      +20     
+ Partials      215      212       -3     
Impacted Files Coverage Δ
pkg/server/deploy/deploy.go 74.28% <73.33%> (+0.41%) ⬆️
pkg/server/k8s/client.go 12.11% <100.00%> (-1.76%) ⬇️

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 31c8d09...93d1a28. Read the comment docs.

@retpolanne retpolanne merged commit 5393e78 into luizalabs:master Jun 17, 2020
@retpolanne retpolanne deleted the fix-636 branch June 17, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404s when using nginx ingress
3 participants