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

fix(builder) set strip_path after defaults #596

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Feb 23, 2022

Edit: the original rationale was wrong, I didn't realize we needed to re-add the mergo replace to KIC modules. This is now only the test.

Kong/kubernetes-ingress-controller#2286 failed when trying to add v1.11.0 to KIC. Something appears to have happened after I tested with #573.

Within ingresRoute, the value is false as expected after it's set to the result of getStripPathBasedOnProtocols(). It flips back to true after https://github.com/Kong/deck/blob/v1.11.0/file/builder.go#L786

I thought this maybe meant something didn't work as expected with #580, but as these were already *bool any non-nil value is not zero, so I'm not sure why the default was merging over the original value.

The change here does fix the issue, but I'd like to better understand what's going on here if possible.

@codecov-commenter
Copy link

Codecov Report

Merging #596 (cec1ac2) into main (4235c73) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
+ Coverage   50.39%   50.46%   +0.07%     
==========================================
  Files          74       74              
  Lines        8277     8279       +2     
==========================================
+ Hits         4171     4178       +7     
+ Misses       3728     3725       -3     
+ Partials      378      376       -2     
Impacted Files Coverage Δ
file/builder.go 56.62% <100.00%> (+0.81%) ⬆️

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 4235c73...cec1ac2. Read the comment docs.

@hbagdi hbagdi requested a review from GGabriele February 24, 2022 14:37
@rainest rainest merged commit 9892fdb into main Feb 24, 2022
@rainest rainest deleted the fix/grpc-defaulter branch February 24, 2022 16:43
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.

3 participants