-
Notifications
You must be signed in to change notification settings - Fork 264
fix: missing trusted_addresses #900
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
Conversation
https://github.com/apache/apisix/pull/12551/files. The breaking change is introduced here, but it's not addressed in the helm chart
|
Waiting for approval on this PR too! |
bzp2010
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version in Chart.yaml should also be updated for automated release.
| {{- end }} | ||
| trusted_addresses: {{ toYaml .Values.apisix.trusted_addresses | nindent 8 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure that this can be applied to a YAML list including multiple CIDRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no activity today I'll update with the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to add an empty default value and some description/documentation in the values.yaml file.
Also, try to follow Helm naming conventions for values (camelCase instead of snake_case): change it to trustedAddresses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey folks, apologies for the delay, I can address the changes in few hours
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right then, we now have two related PRs. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 let's just merge the one which is ready then
https://github.com/apache/apisix/pull/12551/files. The breaking change is introduced here, but it's not addressed in the helm chart