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.

Sign up for GitHub

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

Jump to bottom

Add option for Ingress PathType #634

Merged
merged 9 commits into from
Oct 25, 2021
Merged

Add option for Ingress PathType #634

merged 9 commits into from
Oct 25, 2021

Conversation

tico24
Copy link
Contributor

@tico24 tico24 commented Oct 22, 2021 •
edited
Loading

Signed-off-by: Tim Collins tim@thecollins.team

Fixes: #633

@tico24
Add option for Ingress PathType
fb0eb40 
Signed-off-by: Tim Collins <tim@thecollins.team>
@hashicorp-cla
Copy link

hashicorp-cla commented Oct 22, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tico24
Looks like hashicorp handle the Chart versioning
b7cb5db 
Signed-off-by: Tim Collins <tim@thecollins.team>
@tvoran
Copy link
Member

tvoran commented Oct 22, 2021

Thanks for the quick fix! Would you mind adding tests for this in test/unit/server-ingress.bats?

@tico24
PathType unit test
739db2b 
Signed-off-by: Tim Collins <tim@thecollins.team>
@tico24
Add negative unit test. Add missing char
1123995 
Signed-off-by: Tim Collins <tim@thecollins.team>
@tico24
Copy link
Contributor Author

tico24 commented Oct 25, 2021

Thanks for the quick fix! Would you mind adding tests for this in test/unit/server-ingress.bats?

First time using bats (or frankly, writing unit tests!). Hopefully what I've done is sufficient @tvoran?

@tico24
It seems that I don't need to set the ingressClassName for unit tests
ce8afdd 
Signed-off-by: Tim Collins <tim@thecollins.team>
benashz
benashz requested changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few minor suggestions.

test/unit/server-ingress.bats Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
test/unit/server-ingress.bats Show resolved Hide resolved
test/unit/server-ingress.bats Outdated Show resolved Hide resolved
test/unit/server-ingress.bats Show resolved Hide resolved
tico24 and others added 4 commits October 25, 2021 15:08
@tico24 @benashz
Update test/unit/server-ingress.bats
80f60a2 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@tico24 @benashz
Update values.yaml
4cee00c 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@tico24 @benashz
Update test/unit/server-ingress.bats
09df989 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@tico24
Add >1.19 test and address feedback
9e30da6 
Signed-off-by: Tim Collins <tim@thecollins.team>
@tico24
Copy link
Contributor Author

tico24 commented Oct 25, 2021

All changes implemented @benashz

@tico24 tico24 requested a review from benashz October 25, 2021 14:21
@benashz benashz added the regression fix Fixes a regression bug. label Oct 25, 2021
benashz
benashz approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

LGTM!

@benashz benashz merged commit c09c50f into hashicorp:main Oct 25, 2021
@imthaghost imthaghost mentioned this pull request Oct 25, 2021
Merged
@tvoran tvoran mentioned this pull request Jan 6, 2022
Closed
illegalnumbers pushed a commit to streamnative/vault-helm that referenced this pull request Mar 17, 2022
@tico24 @illegalnumbers
Add option for Ingress pathType (hashicorp#634)
4205f74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benashz benashz benashz approved these changes

Assignees
No one assigned
Labels
regression fix Fixes a regression bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ingress doesn't work with networking.k8s.io/v1 - no pathType set
4 participants
@tico24 @hashicorp-cla @tvoran @benashz